aptos-core icon indicating copy to clipboard operation
aptos-core copied to clipboard

[Move] Allow using all valid structs as transaction arguments

Open banool opened this issue 2 years ago • 7 comments

🚀 Feature Request

Note: This is not a priority right now, we likely won't look at this until after mainnet launch.

As it is now only a limited set of structs are allowed to be used as arguments in transactions. If you define your own struct that you'd like to use as a transaction argument (to a public(script) function), you cannot (see https://github.com/aptos-labs/aptos-core/issues/1886). Right now you instead have to do some combination of writing another function in your move module that can build your struct and return it / use a script transaction / write a wrapper function that takes in the unpacked fields of your struct (e.g. https://github.com/banool/aptos-infinite-jukebox/blob/4178d6a88b63dd33775649ea4ea16e55ba6e619d/move_module/sources/AptosInfiniteJukebox.move#L111).

It would be nice to allow users to just build the struct they want to build locally and then include those in the transaction. This should only be allowed where valid, e.g. when the struct isn't a capability + it is copy. When exactly this is valid needs to be researched further.

banool avatar Jul 11 '22 22:07 banool

This will greatly improve code tidinesses, happy this is in the pipeline

mgild avatar Jul 15 '22 14:07 mgild

+1 for this. Would greatly speed code development to be able to serialize JSON objects that can be deserialized into Move structs. Obviously - this would have to be limited to structs that have copy ability. I would see this working where the SDK provided serialization of a JSON object, pass to the entry function as a vector, then deserialize into the struct type identified by the Move function definition. I would think the safety checking for copy ability would have to happen at run time when deserializing. I don't think a compile time check works as you could have entry functions intended to be called from other on chain modules where passing structs is legitimate. Similar idea to invalid signature error throwing when an entry function with a return value is called from CLI/SDK.

magnum6actual avatar Oct 20 '22 14:10 magnum6actual

I would add - the code for this is already sitting somewhere in this repo (I don't venture beyond aptos-move/framework so not sure where this would be). For the SDK function getTableItem we can already pass a struct for key as a JSON object from an off-chain, client call. So that's being serialized to the transaction and deserialized on chain as an instance of <K> in table<K,V>. It's really just a matter of repurposing that code to allow eligible structs to be passed as arguments from the API.

magnum6actual avatar Oct 21 '22 20:10 magnum6actual

This is not about code sitting somewhere but security of the Move language. Let's say you have a struct like struct SignerCapability { account: address }. There is no direct public constructor for this struct. One can only construct an instance of this struct following a particular access control flow.

But if we allow this struct to be passed in as a parameter into a transaction, everyone from the outside can fake this highly security critical value.

In order to allow structs as parameters, we need to come up with ways how to identify structs which are allowed and which are not as parameters, and/or come up with a generic validation mechanism. It will happen eventually but is not that simple as it looks like.

wrwg avatar Oct 22 '22 03:10 wrwg

Tracking. I would think you could 1) limit to only accepting struct arguments in entry functions in modules that own that struct and 2) treat the passing of the struct the same as if a new instance was created. For example, if you have struct:

struct Example has key {
  prop1: u64,
  prop2: u64
}

Then in terms of Move safety within the same module, this function:

public entry fun pass(value: Example) {
   ...more code

is no different than this function:

public entry fun pass(prop1: u64, prop2: u64) {
  let value = Example{prop1, prop2};
  ...more code

except that one of them is easier to code, especially for larger structures. One Move resource starts its lifecycle in the parameters of the function, one starts its lifecycle in the first line of code of the function.

magnum6actual avatar Oct 22 '22 05:10 magnum6actual

Only allowing structs as parameters for entry functions from the same module is an interesting shortcut to allow some of the desired scenarios here. There are other ideas as well. I was not saying that we won't have this feature, just that it is not that trivial to build.

wrwg avatar Nov 03 '22 13:11 wrwg

All good - you guys have your hands full right now. I told Santa all I want this year for Christmas is custom structs in entry functions! :)

magnum6actual avatar Nov 03 '22 13:11 magnum6actual

This issue is stale because it has been open 45 days with no activity. Remove the stale label or comment - otherwise this will be closed in 15 days.

github-actions[bot] avatar Dec 19 '22 01:12 github-actions[bot]

About time to open this one back up, yes? We have so much structured data we need to pass to applications that I'm going bananas with flat parameters.

magnum6actual avatar Jun 05 '23 02:06 magnum6actual

Not sure whether we need a generic bug like this. We certainly wont open up for every struct because this causes security issues, as it can violate invariants which are expected to hold for structs when those can be created out of thin air. At the same time we are fully aware of the problem and incrementally open up what we consider secure. See e.g. this newer issue: https://github.com/aptos-labs/aptos-core/issues/8711

We also have the medium term goal to extend the Move language by the concept of public/open structs, which would then also be feasible for this.

wrwg avatar Jun 17 '23 04:06 wrwg

The idea of public structs is great. That would be exactly what I'm looking for here. Will keep an eye out for that one.

magnum6actual avatar Jun 18 '23 03:06 magnum6actual