Implement SNIP-9 #1111

kfastov · 2024-05-01T14:03:36Z

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 · 2024-05-01T14:09:03Z

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 · 2024-05-02T12:56:55Z

thanks!! we will take a deep look!

tabaktoni · 2024-05-03T10:56:22Z

We should add documentation for OutsideExecution usage

tabaktoni

In general lgtm.

docs
e2e account test

tabaktoni · 2024-05-03T11:24:39Z

src/types/outsideExecution.ts

+
+export const OutsideExecutionCallerAny = encodeShortString('ANY_CALLER');
+
+export class OutsideExecution {


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.

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

tabaktoni · 2024-05-03T11:29:03Z

__tests__/outsideExecution.ts

+
+    expect(abiData).toEqual(expectedResult);
+  });
+});


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.

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

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?

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 :)

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

ivpavici · 2024-05-17T14:29:10Z

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

kfastov · 2024-05-22T16:47:33Z

@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

ivpavici · 2024-06-18T10:06:20Z

@kfastov hi hi! gentle ping about this PR :)

kfastov · 2024-06-19T18:52:57Z

@ivpavici Thank you for waiting so long 🙏. I'll push the updates tomorrow

kfastov · 2024-06-20T18:50:06Z

Sorry out of time today, but I'll certainly do it on Friday

kfastov · 2024-06-22T18:30:18Z

@ivpavici @tabaktoni I've added docs and changed the file structure to match convention. Can you please review?

penovicp

Pointed out some minor issues, in general lgtm.

There are also four TODOs that appear to mostly be stragglers, they are either done or the mentioned choices seem fine to me.

src/account/default.ts

__tests__/account.outsideExecution.test.ts

__tests__/outsideExecution.ts

www/docs/guides/outsideExecution.md

docs: remove unnecessary section for OutsideExecution guide

kfastov · 2024-06-28T19:16:24Z

@penovicp Can you please check now?

ivpavici · 2024-07-01T08:29:10Z

src/account/default.ts

+    }
+    // if not, return it
+    return nonce;
+    // TODO process errors


@kfastov is this a leftover or?

I'll do couple more checks (suggested by @PhilippeR26 ) and decide if there need to be any error checks here

PhilippeR26

For information, current ArgentX account contract is version v0.4.0.
I have to check if it has impacts on snip-9 implementation.

PhilippeR26 · 2024-07-03T15:59:53Z

Hello,
I am trying to test your code in the guide.
Have you executed it in a node script (importing starknet)?
Because on my side, I am not able to run :

import { OutsideExecution, OutsideExecutionOptions } from 'starknet';

In fact it seems that nothing has been exported to the user level.

PhilippeR26 · 2024-07-04T09:42:04Z

Is your code handling smoothly accounts that are not implementing the ERC165 (introspection)?
You should try with this Testnet account class : 0x2ac408bab4a8b1451fd6395fef68be20484002f2c73faa87bd552b610699bfb

kfastov · 2024-07-05T15:47:38Z

Hello, I am trying to test your code in the guide. Have you executed it in a node script (importing starknet)? Because on my side, I am not able to run :
import { OutsideExecution, OutsideExecutionOptions } from 'starknet';
In fact it seems that nothing has been exported to the user level.

Oops, my bad, I'll fix it now

docs: update documentation to match the code

PhilippeR26 · 2024-07-15T18:47:41Z

@kfastov Have you finalized your modifications ?

PhilippeR26

I didn't checked the code, the test & the doc.
I focused on my own tests and the resulting UX.
In most cases, it works, but this case is still crashing : #1111 (comment) (in Account.getSnip9Version()).
I tested the following accounts :

ArgentX 0.4.0 : v2
Braavos 1.0.0 : v2
ArgentX 0.3.1 : v1
OpenZeppelin 0.14.0 : not supported
special : ERC165 & SNIP-9 not supported.

If you want to play with them : https://github.com/PhilippeR26/starknet.js-workshop-typescript/blob/main/src/scripts/Starknet131/Starknet131-devnet/16.ExecuteFromOutside.ts

Nevertheless, I am not happy about the user experience ; it's really not straightforward and pleasant. I think that this new OutsideExecution class is the origin of the problem.
I tested several things, and I think that we are able to have a such code on user side, with many things calculated automatically in the Account class :

// option has no nonce ; it will be generated automatically.
const options: OutsideExecutionOptions = {
    caller: "ANY_CALLER", // and not outsideExecution.OutsideExecutionCallerAny
    execute_after: Math.floor(Date.now() / 1000) - 3600, // 1 hour ago
    execute_before: Math.floor(Date.now() / 1000) + 3600, // 1 hour from now
  };
const call1 = {
      contractAddress: ethAddress,
      entrypoint: 'transfer',
      calldata: {
        recipient: account1.address,
        amount: cairo.uint256(10n ** 15n),
      },
};
const call2 = {
      contractAddress: ethAddress,
      entrypoint: 'transfer',
      calldata: {
        recipient: account2.address,
        amount: cairo.uint256(2n * 10n ** 15n),
      },
};
// account A is compatible with SNIP-9
const outsideTransaction1 = await accountA.getOutsideTransaction(options, call1);
const outsideTransaction2 = await accountA.getOutsideTransaction(options, call2);
// accountB can be not compatible with SNIP-9
// no mandatory order to proceed
const res0 = accountB.executeFromOutside(outsideTransaction2, addressOfAccountA);
await myProvider.waitForTransaction(res0.transaction_hash);
const res1 = accountB.executeFromOutside(outsideTransaction1, addressOfAccountA);
await myProvider.waitForTransaction(res1.transaction_hash);

You can see as easy it can be.
Please consider to change the Account class in accordance to this proposal.

PS : getSnip9Version() should respond "UNSUPPORTED", and not "0" as currently.

kfastov added 2 commits May 1, 2024 14:10

feat: draft SNIP-9 implementation

002b40a

refactor: implement more SNIP-9 methods

3f4ba15

ivpavici requested review from tabaktoni and dhruvkelawala May 2, 2024 12:58

tabaktoni added the Type: feature New feature or request label May 3, 2024

tabaktoni requested changes May 3, 2024

View reviewed changes

test: add SNIP-9 tests, fix impl

e0be3b9

tabaktoni changed the base branch from develop to next-version June 12, 2024 11:12

tabaktoni added the pending need update label Jun 12, 2024

penovicp deleted the branch starknet-io:develop June 21, 2024 04:26

penovicp closed this Jun 21, 2024

penovicp reopened this Jun 21, 2024

penovicp changed the base branch from next-version to develop June 21, 2024 04:31

kfastov added 4 commits June 22, 2024 16:35

chore: move outsideExecution class to utils

aa8f23e

chore(utils): merge snip9.ts with outsideExecution.ts

6113a63

chore: remove barrel file from types because of convention violation

182519c

docs: added guide for outside execution

daf2347

kfastov requested a review from tabaktoni June 22, 2024 18:25

ivpavici requested a review from penovicp June 24, 2024 08:01

penovicp reviewed Jun 24, 2024

View reviewed changes

src/account/default.ts Outdated Show resolved Hide resolved

__tests__/account.outsideExecution.test.ts Outdated Show resolved Hide resolved

__tests__/outsideExecution.ts Outdated Show resolved Hide resolved

www/docs/guides/outsideExecution.md Outdated Show resolved Hide resolved

chore: remove irrelevant leftover comments and code

d5af037

docs: remove unnecessary section for OutsideExecution guide

penovicp approved these changes Jul 1, 2024

View reviewed changes

ivpavici reviewed Jul 1, 2024

View reviewed changes

ivpavici requested a review from PhilippeR26 July 1, 2024 08:33

ivpavici removed the pending need update label Jul 1, 2024

ivpavici changed the title ~~Implement SNIP-9 (WiP)~~ Implement SNIP-9 Jul 1, 2024

PhilippeR26 reviewed Jul 1, 2024

View reviewed changes

test: upgrade Argent account artifacts

1e6c8c0

ivpavici requested a review from PhilippeR26 July 10, 2024 12:45

kfastov added 2 commits July 14, 2024 12:42

fix: export OutsideExecution types

ba93be6

refactor: change outside execution version type

0f52622

docs: update documentation to match the code

ivpavici assigned tabaktoni and PhilippeR26 Jul 15, 2024

PhilippeR26 requested changes Jul 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement SNIP-9 #1111

Implement SNIP-9 #1111

kfastov commented May 1, 2024 •

edited

Loading

kfastov commented May 1, 2024 •

edited

Loading

ivpavici commented May 2, 2024

tabaktoni commented May 3, 2024

tabaktoni left a comment

tabaktoni May 3, 2024

kfastov May 5, 2024 •

edited

Loading

tabaktoni May 3, 2024

kfastov May 5, 2024

kfastov May 7, 2024

ivpavici May 8, 2024 •

edited

Loading

vladutjs May 9, 2024

ivpavici commented May 17, 2024

kfastov commented May 22, 2024

ivpavici commented Jun 18, 2024

kfastov commented Jun 19, 2024 •

edited

Loading

kfastov commented Jun 20, 2024

kfastov commented Jun 22, 2024

penovicp left a comment

kfastov commented Jun 28, 2024

ivpavici Jul 1, 2024

kfastov Jul 5, 2024

PhilippeR26 left a comment

PhilippeR26 commented Jul 3, 2024 •

edited

Loading

PhilippeR26 commented Jul 4, 2024

kfastov commented Jul 5, 2024

PhilippeR26 commented Jul 15, 2024

PhilippeR26 left a comment


		export const OutsideExecutionCallerAny = encodeShortString('ANY_CALLER');

		export class OutsideExecution {

Implement SNIP-9 #1111

Are you sure you want to change the base?

Implement SNIP-9 #1111

Conversation

kfastov commented May 1, 2024 • edited Loading

Motivation and Resolution

RPC version (if applicable)

Usage related changes

Development related changes

Checklist:

Things left:

kfastov commented May 1, 2024 • edited Loading

ivpavici commented May 2, 2024

tabaktoni commented May 3, 2024

tabaktoni left a comment

Choose a reason for hiding this comment

tabaktoni May 3, 2024

Choose a reason for hiding this comment

kfastov May 5, 2024 • edited Loading

Choose a reason for hiding this comment

tabaktoni May 3, 2024

Choose a reason for hiding this comment

kfastov May 5, 2024

Choose a reason for hiding this comment

kfastov May 7, 2024

Choose a reason for hiding this comment

ivpavici May 8, 2024 • edited Loading

Choose a reason for hiding this comment

vladutjs May 9, 2024

Choose a reason for hiding this comment

ivpavici commented May 17, 2024

kfastov commented May 22, 2024

ivpavici commented Jun 18, 2024

kfastov commented Jun 19, 2024 • edited Loading

kfastov commented Jun 20, 2024

kfastov commented Jun 22, 2024

penovicp left a comment

Choose a reason for hiding this comment

kfastov commented Jun 28, 2024

ivpavici Jul 1, 2024

Choose a reason for hiding this comment

kfastov Jul 5, 2024

Choose a reason for hiding this comment

PhilippeR26 left a comment

Choose a reason for hiding this comment

PhilippeR26 commented Jul 3, 2024 • edited Loading

PhilippeR26 commented Jul 4, 2024

kfastov commented Jul 5, 2024

PhilippeR26 commented Jul 15, 2024

PhilippeR26 left a comment

Choose a reason for hiding this comment

kfastov commented May 1, 2024 •

edited

Loading

kfastov commented May 1, 2024 •

edited

Loading

kfastov May 5, 2024 •

edited

Loading

ivpavici May 8, 2024 •

edited

Loading

kfastov commented Jun 19, 2024 •

edited

Loading

PhilippeR26 commented Jul 3, 2024 •

edited

Loading