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

Implement SNIP-9 (WiP) #1111

Open
wants to merge 3 commits into
base: next-version
Choose a base branch
from
Open

Conversation

kfastov
Copy link

@kfastov kfastov commented May 1, 2024

Motivation and Resolution

This PR implements the SNIP-9 (outside execution) standard in starknet.js. SNIP-9 enables protocols to submit transactions on behalf of a user account, as long as they have the relevant signatures. This feature provides flexibility for various use cases such as delayed orders and fee subsidies.

The implementation follows the SNIP-9 specification and introduces new types, utilities, and account methods to support off-chain signatures. It includes the OutsideExecution class, which represents an outside transaction to be executed on behalf of the user account. The Account class has been extended with methods to get the supported SNIP-9 version, check nonce validity, and execute an outside execution.

RPC version (if applicable)

N/A

Usage related changes

  • Added support for SNIP-9 outside execution in starknet.js.
  • Introduced new types related to SNIP-9, including OutsideExecution, OutsideCall, and EOutsideExecutionVersion.
  • Extended the Account class with methods to support SNIP-9:
    • getSnip9Version: Retrieves the supported SNIP-9 version of the account.
    • isValidSnip9Nonce: Checks if a given nonce is valid for the account.
    • executeFromOutside: Executes an outside execution on behalf of the account.

Development related changes

  • Added new utility functions for SNIP-9, such as buildExecuteFromOutsideCallData() and getSnip9Nonce().
  • Updated the documentation to include information about SNIP-9 and the new account methods.

Checklist:

  • Performed a self-review of the code
  • Rebased to the last commit of the target branch (or merged it into my branch)
  • Linked the issues which this PR resolves
  • Documented the changes in code (API docs will be generated automatically)
  • Updated the tests
  • All tests are passing

Things left:

  • I haven't covered the entire flow in the tests yet
  • There is a bug in fee estimation, and the current implementation has maxFee hardcoded, which should be changed
  • Not every function/method is documented yet

Fixes #948

@kfastov
Copy link
Author

kfastov commented May 1, 2024

To explain my choice of hashing algorithms:

In the SNIP-9 specification it's stated that SNIP-12 should be used for hashing. There are 2 revisions of SNIP-12 that use different hashing algorithms. Further in the section "For account builders" it says that version 1 of SNIP-9 implements revision 0 of SNIP-12, and version 2 implements revision 1. Studying the code of actual contracts confirms it. Current Argent account contract implements SNIP-9 v1 with SNIP-12 revision 0, and Braavos implements SNIP-9 v2 with SNIP-12 revision 1. Since Starknet.js itself supports both SNIP-12 revisions, I've added support for both SNIP-9 versions, using version parameter to determine the hash algorithm being used, and auto-determining the version where possible

@ivpavici
Copy link
Collaborator

ivpavici commented May 2, 2024

thanks!! we will take a deep look!

@tabaktoni tabaktoni added the Type: feature New feature or request label May 3, 2024
@tabaktoni
Copy link
Collaborator

We should add documentation for OutsideExecution usage

Copy link
Collaborator

@tabaktoni tabaktoni left a comment

Choose a reason for hiding this comment

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

In general lgtm.

  • docs
  • e2e account test


export const OutsideExecutionCallerAny = encodeShortString('ANY_CALLER');

export class OutsideExecution {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We used (because of initial setup) types dir for pure type definitions and utils for implementations.
I think this is fine but it will break existing convenience.

Copy link
Author

@kfastov kfastov May 5, 2024

Choose a reason for hiding this comment

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

Yeah I will leave the interface inside types and move the class and implementation into utils, if it's fine


expect(abiData).toEqual(expectedResult);
});
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there an Acc implementing this snip so we can test it on Account?
If so I would like to have a full e2e test with ACC using it.

Copy link
Author

Choose a reason for hiding this comment

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

Yep, Argent and Braavos support it at least on Sepolia, I'll write the missing test in a few days, sorry for the delay

Copy link
Author

Choose a reason for hiding this comment

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

I just realized that OZ account contract doesn't support SNIP-9 yet. And both Argent and Braavos are distributed under GPL, so I can't just take their compiled contracts and put them into __mocks__ directory. I can request a feature on OZ but nobody knows how long it will take. If I compile a "dummy" account contract with minimal execute_from_outside implementation and use it in tests, will it count as a valid test?

Copy link
Collaborator

@ivpavici ivpavici May 8, 2024

Choose a reason for hiding this comment

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

Tagging Argent folks, @janek26 @dhruvkelawala hi! for testing purposes, does GPL license allow usage of your Account contract for testing this SNIP? @kfastov I don't think we would have problems with the GPL license, this is a non-profit library in any case :)

Copy link

Choose a reason for hiding this comment

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

@ivpavici all good to use our contracts. double checked it with Sergio.

@ivpavici
Copy link
Collaborator

@kfastov hi! Will you have time to add these couple of things left? 🙏

@kfastov
Copy link
Author

kfastov commented May 22, 2024

@kfastov hi! Will you have time to add these couple of things left? 🙏

Hello! Sorry for being slow, had to make a forced break in development, but now I am here again.
Made small fixes and added a test today. Now working on documentation

@tabaktoni tabaktoni changed the base branch from develop to next-version June 12, 2024 11:12
@tabaktoni tabaktoni added the pending need update label Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending need update Type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for SNIP-9
4 participants