Migrate account #616

ericnordelo · 2023-05-04T15:49:51Z

Fixes #547

PR Checklist

Tests
Tried the feature on a public network
Documentation

Co-authored-by: Andrew Fleming <fleming-andrew@protonmail.com> Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>

andrew-fleming

Excellent work on the account and tests! I left some comments, suggestions, and questions

src/openzeppelin/account.cairo

src/openzeppelin/utils.cairo

src/openzeppelin/account.cairo

src/openzeppelin/tests/test_account.cairo

src/openzeppelin/account.cairo

src/openzeppelin/tests/test_account.cairo

src/openzeppelin/account.cairo

src/openzeppelin/tests/test_account.cairo

martriay · 2023-05-12T05:27:25Z

src/openzeppelin/tests/test_account.cairo

+#[available_gas(2000000)]
+fn test_multicall() {
+    let account = setup_dispatcher(Option::Some(@SIGNED_TX_DATA()));
+    let erc20 = deploy_erc20(account.contract_address, 1000);


if we decide to drop the account deployment, we can simply use a const address for minting, then we can set it as tx sender

Following on this comment, personally, I wouldn't say I like the idea of removing the deployment of the account, to me is getting further away from the actual flow, and in some cases, we can't.

in which cases we can't? agree that we're departing from the flow, but i'm not sure deploying is getting us closer. maybe i'll understand your point by looking at the cases we can't, but otherwise it doesn't seem to be adding much value.

execute as stated in this comment.

IMO the complexity added on top is pretty small, and at least having the entry point executed by a syscall using the setup_dispatcher (setting the contract_address and the caller_address automatically) is less error-prone (even when is still quite far from the real flow, is closer). But there is a line ofc, we can use direct calls (instead of the dispatcher) for simple logic like Account::_is_valid_signature, which is not an entry point or using context, but we need to use dispatcher to test interactions like execute, I cannot say exactly what we should use in the middle, but for me looks better using dispatcher for entry points like validate, even when we could use direct calls.

If you feel we should be using direct calls for this I will update accordingly.

no, it's okay. let's then make this explicit by stating that we deploy contracts when call_contract_syscall is involved since target contracts need to be registered and sometimes to know the caller's address.

similar to what came up in the constructor vs initializer discussion, i think the main distinction between presets and libraries is that we intend to deploy the former and not the latter, so it's ok to test module's functions directly as long as deployment is not needed. therefore i would favor not deploying for all cases but the ones where call_contract_syscall is involved. would you agree with that?

src/openzeppelin/tests/test_account.cairo

martriay

very good job! i subscribe to many of Andrew's comments, adding my own

src/openzeppelin/account.cairo

src/openzeppelin/tests/test_account.cairo

…onals

…o feat/test-account

andrew-fleming

Great work! I left a few questions and comments, but I think we're looking good :)

src/openzeppelin/account.cairo

src/openzeppelin/tests/test_account.cairo

src/openzeppelin/account.cairo

andrew-fleming · 2023-05-16T17:58:32Z

src/openzeppelin/account.cairo

+    //
+
+    #[internal]
+    fn _assert_only_self() {


I'm curious about this as well. Perhaps, saving the underscore function only for internal functions that bear the same name would be appropriate e.g.

#[view] fn is_valid_signature(){} #[internal] fn assert_only_self(){} #[internal] fn _is_valid_signature(){}

IMO it's a simple convention to follow. What do you guys think?

Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>

src/openzeppelin/account.cairo

src/openzeppelin/token/erc20.cairo

src/openzeppelin/tests/test_account.cairo

src/openzeppelin/account.cairo

martriay · 2023-05-17T05:40:38Z

src/openzeppelin/tests/test_account.cairo

+    let account = setup_dispatcher(Option::None(()));
+    let public_key: felt252 = account.get_public_key();
+    assert(public_key == PUBLIC_KEY(), 'Should return public key');


i think simplification can both make tests easier to read and effectively simpler in execution

src/openzeppelin/tests/test_account.cairo

martriay · 2023-05-17T05:50:36Z

src/openzeppelin/tests/test_account.cairo

+#[available_gas(2000000)]
+fn test_multicall() {
+    let account = setup_dispatcher(Option::Some(@SIGNED_TX_DATA()));
+    let erc20 = deploy_erc20(account.contract_address, 1000);


in which cases we can't? agree that we're departing from the flow, but i'm not sure deploying is getting us closer. maybe i'll understand your point by looking at the cases we can't, but otherwise it doesn't seem to be adding much value.

src/openzeppelin/tests/test_account.cairo

martriay · 2023-05-17T06:16:57Z

src/openzeppelin/tests/test_account.cairo

+#[test]
+#[available_gas(2000000)]
+fn test_execute() {
+    test_execute_with_version(Option::None(()));


what about moving the setup_dispatcher version setting in here? or do we need it in other tests as well?

The version is part of the context that setup_dispatcher is initializing to make the calls in tests closer to reality. Is true that we can test validate_deploy (for ex.) without setting the correct transaction version, but IMO this is more error-prone because we can easier forget to set this context on a test that passes because of that. I think setting the basic context in setup_dispatcher, and using setup_dispatcher when possible is a better pattern than direct calls with incorrect context (as before to mention that there is a line, but for me most tests should use setup_dispatcher instead of direct calls)

but isn't tx version only used in __execute__ where tests are already mindful of it? i agree with you in spirit (approximate to reality, minimize error-prone approaches) but in practice we end up:

splitting the version setting between the setup function and these test cases

adding an if in the test body

and we have a quirky Option::None() in here

i don't feel that strongly about this, so if you do then i'm ok with keeping it as it is

…racts into feat/test-account

andrew-fleming

Looking very nice! On top of the open discussions, I just left a few small questions. Also, they recently released a new version of cairo. What do you think about bumping up the cairo submodule to the release commit since it's a bit more meaningful?

src/openzeppelin/account/account.cairo

src/openzeppelin/tests/test_account.cairo

martriay · 2023-05-19T21:23:40Z

src/openzeppelin/tests/test_account.cairo

+#[test]
+#[available_gas(2000000)]
+fn test_execute() {
+    test_execute_with_version(Option::None(()));


but isn't tx version only used in __execute__ where tests are already mindful of it? i agree with you in spirit (approximate to reality, minimize error-prone approaches) but in practice we end up:

splitting the version setting between the setup function and these test cases

adding an if in the test body

and we have a quirky Option::None() in here

i don't feel that strongly about this, so if you do then i'm ok with keeping it as it is

martriay · 2023-05-19T22:58:40Z

src/openzeppelin/tests/test_account.cairo

+#[available_gas(2000000)]
+fn test_multicall() {
+    let account = setup_dispatcher(Option::Some(@SIGNED_TX_DATA()));
+    let erc20 = deploy_erc20(account.contract_address, 1000);


no, it's okay. let's then make this explicit by stating that we deploy contracts when call_contract_syscall is involved since target contracts need to be registered and sometimes to know the caller's address.

similar to what came up in the constructor vs initializer discussion, i think the main distinction between presets and libraries is that we intend to deploy the former and not the latter, so it's ok to test module's functions directly as long as deployment is not needed. therefore i would favor not deploying for all cases but the ones where call_contract_syscall is involved. would you agree with that?

src/openzeppelin/account/account.cairo

src/openzeppelin/tests/test_account.cairo

src/openzeppelin/account/interface.cairo

martriay · 2023-05-23T21:02:57Z

Superseded by #620. Thanks @ericnordelo for the great work!

martriay and others added 30 commits March 7, 2023 21:03

Merge branch 'cairo-1' into erc165

c6996fa

Merge branch 'erc165' into account

387637e

Merge branch 'cairo-1' into account

90a7a4b

Merge branch 'cairo-1' into erc165

f66a988

continue account implementation

e2d7248

add missing account interface functions

52019ac

tidy up module

50b96bf

fix validate

aae6451

Merge branch 'cairo-1' into account

4974643

bump cairo + account changes

c485e60

fix __execute__, add serde, rename felt>felt252

921e61f

tidy up code

fc2d995

add account tests

f756504

complete account implementation

ec321c0

Apply suggestions from code review

27b0f7e

Co-authored-by: Andrew Fleming <fleming-andrew@protonmail.com> Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>

apply review suggestions

f2a43f5

Merge branch 'cairo-1' into erc165

7e5b91b

remove unused import

cbfb2dd

merge erc165 changes

2b9cfd7

clarify __execute__ guard

2111a20

add account tests

bd5979e

add internals

995fadd

tidy up

c2d4721

Merge branch 'cairo-1' into erc165

1b9b311

add internal macro

52357cc

Merge branch 'erc165' into account

c615ca3

add internal macro

cee8f05

add deregister

d2ddedf

Merge branch 'erc165' into account

a65e1ef

update erc165

2e859a4

andrew-fleming reviewed May 11, 2023

View reviewed changes

martriay reviewed May 12, 2023

View reviewed changes

src/openzeppelin/account.cairo Outdated Show resolved Hide resolved

src/openzeppelin/account.cairo Outdated Show resolved Hide resolved

src/openzeppelin/tests/test_account.cairo Show resolved Hide resolved

src/openzeppelin/tests/test_account.cairo Outdated Show resolved Hide resolved

archseer mentioned this pull request May 12, 2023

Cairo 1.0 smartcontractkit/chainlink-starknet#224

Merged

ericnordelo added 5 commits May 12, 2023 16:52

refactor: apply review comments

f51c1d7

test: execute out of gas

4db1f2f

refactor: is valid signature because of lack of short circuit condici…

bce4724

…onals

refactor: apply updates from reviews

c7e18db

Merge branch 'cairo-1' of github.com:OpenZeppelin/cairo-contracts int…

b9c3051

…o feat/test-account

ericnordelo requested review from andrew-fleming and martriay May 15, 2023 13:11

ericnordelo added 2 commits May 15, 2023 16:40

feat: add SpanSerde implementation

d94c8b1

refactor: remove span_to_array

04ed19c

andrew-fleming reviewed May 16, 2023

View reviewed changes

ericnordelo and others added 2 commits May 16, 2023 23:32

Update src/openzeppelin/tests/test_account.cairo

18f3cb5

Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>

Update src/openzeppelin/account.cairo

27280f6

Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>

martriay reviewed May 17, 2023

View reviewed changes

ericnordelo added 4 commits May 17, 2023 15:16

test: update from review

00b4110

feat: splitting account interface and abi

98bf7ef

Merge branch 'feat/test-account' of github.com:ericnordelo/cairo-cont…

630e300

…racts into feat/test-account

feat: add new account files

164b962

ericnordelo requested review from martriay and andrew-fleming May 17, 2023 15:48

andrew-fleming reviewed May 18, 2023

View reviewed changes

src/openzeppelin/account/account.cairo Show resolved Hide resolved

src/openzeppelin/account/account.cairo Show resolved Hide resolved

src/openzeppelin/tests/test_account.cairo Show resolved Hide resolved

src/openzeppelin/tests/test_account.cairo Show resolved Hide resolved

martriay reviewed May 19, 2023

View reviewed changes

martriay changed the title ~~Migrate account with new test features~~ Migrate account May 23, 2023

andrew-fleming mentioned this pull request May 23, 2023

Migrate account #620

Merged

3 tasks

martriay closed this May 23, 2023

ericnordelo deleted the feat/test-account branch September 22, 2023 13:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate account #616

Migrate account #616

ericnordelo commented May 4, 2023 •

edited

Loading

andrew-fleming left a comment

martriay May 12, 2023

ericnordelo May 15, 2023

martriay May 17, 2023

ericnordelo May 17, 2023

ericnordelo May 17, 2023 •

edited

Loading

martriay May 19, 2023

martriay left a comment

andrew-fleming left a comment

andrew-fleming May 16, 2023

martriay May 17, 2023

martriay May 17, 2023

martriay May 17, 2023

ericnordelo May 17, 2023

martriay May 19, 2023

andrew-fleming left a comment

martriay May 19, 2023

martriay May 19, 2023

martriay commented May 23, 2023

Migrate account #616

Migrate account #616

Conversation

ericnordelo commented May 4, 2023 • edited Loading

PR Checklist

andrew-fleming left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericnordelo May 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martriay left a comment

Choose a reason for hiding this comment

andrew-fleming left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew-fleming left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martriay commented May 23, 2023

ericnordelo commented May 4, 2023 •

edited

Loading

ericnordelo May 17, 2023 •

edited

Loading