Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add multicall #73

Closed
wants to merge 1 commit into from
Closed

Add multicall #73

wants to merge 1 commit into from

Conversation

martriay
Copy link
Contributor

Since this feature requires a list of messages [Message] and each Message contains a list of arguments as calldata, it cannot be completed until Cairo supports passing an array of structs or at a least nested array simulating one to external functions.

@martriay martriay marked this pull request as draft November 18, 2021 05:43
@martriay
Copy link
Contributor Author

Fixes #20.

calldata: felt*,
nonce: felt
) -> (response : felt):
messages_len: felt,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should introduce a new Call struct containing only the to selector and calldata to avoid passing the sender and nonce which are global to all the calls, e.g:

struct Call:
   member to: felt
   member selector: felt
   member calldata_size: felt
   member calldata: felt*
end

and pass an array of Call to the execute method.

Copy link
Contributor Author

@martriay martriay Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my idea with the new Message structure that dropped sender but I didn't realize it can also drop the nonce, great call! (pun intended).

This was a draft only and it got stalled due to the reasons explained above but my idea was to eventually open a github discussion about it. Now that I have you here: what do you think about having a single entrypoint?

Pros:

  • less code
  • easier to reason about
  • easier to implement

Cons:

  • we'd be moving away from the interface agreed on Standard account interface #41
  • I don't like the feel of having such a complex structure (array of structs) as the main standard interface, but can't really explain why

The alternative would be to keep the agreed interface and just add a multi_execute method or something like that, although it's added complexity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we by chance decided to go for a new interface, we should push for it asap to make the relevant changes across tools (Signer.py, starknet.js, etc) while the ecosystem is still small. If that's the case, we would also need to push for Cairo to provide the required features.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That or just add a separate method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I like the idea of having a single entry point. I find it cleaner, but also I would find it weird to have 2 different methods to achieve the same thing (1 single call). And maybe it will make it easier to deal with fees when that's added to StarkNet.

Changing the Account interface is fine since we are still very early. But we should check with the Starkware team when Cairo will support arrays of structs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it would be interesting to understand the overhead of doing a single call when there is one entry point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. I let the starware team know this is blocking the standard. I'm under the impression that this interface will work and that if there's such an overhead, it can be worked out although input from the SW team would also be appreciated.

@bbrandtom
Copy link

Thanks, added it to the features list. Hopefully will be added to the roadmap soon/
An array of structs will probably be easier to add, is this the preferred solution?

@martriay
Copy link
Contributor Author

Thanks, added it to the features list. Hopefully will be added to the roadmap soon/ An array of structs will probably be easier to add, is this the preferred solution?

Yes I'd rather have an array of structs instead of an array of arrays, otherwise I need to unpack the array into the struct which is added complexity, increasing the bug surface area, making it harder to read and audit.

@bbrandtom
Copy link

Arrays of structs working internally :-)
Kudos to LiorG

@juniset
Copy link
Contributor

juniset commented Feb 1, 2022

@martriay @andrew-fleming Since passing array of structs containing pointers to external methods is not implemented in Cairo 0.7.0 and there is no ETA yet on when it will be supported, I've implemented a (intermediary) solution that works today https://github.com/argentlabs/argent-contracts-starknet/blob/6d3c570fc1896b0c0f380bd8fb580fb1c9f7d0ac/contracts/ArgentAccount.cairo#L143-L165

It leverages the fact that they can be passed internally so all the internal methods to compute the hash, etc, already uses the target Call struct, only the execute method does not.

Should we try to push that to the IAccount interface (and the OZ and Argent implementations) now so that dapp developers can start playing with multicalls early? It will still require 1 or 2 iterations to reach the final interface; one to add fees (~3-4 weeks according to Starkware) and one to enable the proper array of structs (no ETA), but most of these changes can be abstracted to dapp developers by embedding them in starknet.js. What do you think?

@martriay
Copy link
Contributor Author

martriay commented Feb 23, 2022

Superseded by #192

@martriay martriay closed this Feb 23, 2022
@martriay martriay deleted the multicall branch February 23, 2022 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants