Migrate Account to component #783

ericnordelo · 2023-10-10T18:39:59Z

Fixes #782

PR Checklist

Tests
Tried the feature on a public network
Documentation

docs/modules/ROOT/pages/api/account.adoc

andrew-fleming

Looking good, Eric! I left a few comments and suggestions

Also, the __execute__ method in the API should have self as a snapshot

src/account/account.cairo

docs/modules/ROOT/pages/api/account.adoc

src/tests/account/test_account.cairo

src/tests/account/test_dual_account.cairo

docs/modules/ROOT/pages/api/account.adoc

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

…eat/migrate-account-to-component

…rdelo/cairo-contracts into feat/migrate-account-to-component

…eat/migrate-account-to-component

andrew-fleming

LGTM!

src/account/account.cairo

src/tests/account/test_dual_account.cairo

src/tests/mocks/account_mocks.cairo

martriay · 2023-10-23T00:02:10Z

docs/modules/ROOT/pages/index.adoc

+    component!(path: account_component, storage: account, event: AccountEvent);
+    component!(path: src5_component, storage: src5, event: SRC5Event);


should each of these be next of their impls?

I think is easier to read with the component! macro calls on top, because you can quickly get what components are being used. I don't think there's much value on moving them next to the impls, but no strong opinion.

agree on making components easy to understand at a quick glance. Just to counterbalance, the value I see is grouping logically related lines, making it easy to:

add/remove components without affecting all lines

make sure all directives for a component are in place (event, component, importing impl)

performing reviews gets simpler and easier, based on the above items

docs/modules/ROOT/pages/api/account.adoc

Co-authored-by: Martín Triay <martriay@gmail.com>

…rdelo/cairo-contracts into feat/migrate-account-to-component

…eat/migrate-account-to-component

ericnordelo · 2023-10-25T11:26:11Z

I added a preset with tests, maybe it is too much for just one PR. Let me know if I should better split it. Documenting the preset in the doc-site API corresponding section is still missing.

andrew-fleming

Looking good, Eric! I left some comments and questions

I added a preset with tests, maybe it is too much for just one PR. Let me know if I should better split it. Documenting the preset in the doc-site API corresponding section is still missing.

Yeah, I'd split the presets into another PR so discussions on that don't block this getting merged. The more tight and focused the PR, the better IMO

andrew-fleming · 2023-10-25T17:44:02Z

src/account/presets/account.cairo

+///
+/// Openzeppelin's account contract.
+#[starknet::contract]
+mod Account {


I don't love the Account preset name. I guess we can differentiate between the two Accounts with component and contract, but idk. AccountPreset maybe or something better? Thoughts?

Agree, but i'd go with BasicAccount or something, to distinguish from future EthereumAccount, or MultisigAccount presets. I'd never call "preset" a contract btw.

FWIW, I rather keep the name clean as Account, because the separation exists in the directory (imports) structure, and you cannot use a component as a preset, or a preset as a component (compiler error), so the potential issues from this clash are minimal IMO.

For Account we can come up with a name, but what about ERC20? Which names would you use for the preset? Since we are probably removing presets in favor of Wizard anyway, I vote for keeping the name as just Account (and ERC20 when it comes the time).

Let me know if I still should update it, and to what name then.

we can keep it like this for the time being, we can reassess post wizard

src/tests/account/test_dual_account.cairo

src/tests/presets/test_account.cairo

andrew-fleming · 2023-10-25T20:26:26Z

src/tests/presets/test_account.cairo

+use openzeppelin::tests::account::test_account::{
+    deploy_erc20, SIGNED_TX_DATA, SignedTransactionData, TRANSACTION_VERSION, QUERY_VERSION
+};


I vote we import TRANSACTION_VERSION and QUERY_VERSION from the account component and not the test mod

So I'm wondering if we should move presets/ into the component test directory. My thinking is that we could create a component-specific utils file and import deploy_erc20, SIGNED_TX_DATA, and SignedTransactionData from it (since we're reusing these with accounts). One concern I have, though, is that presets may not have a long future because of the wizard. Maybe it's not worth the trouble, idk. Thoughts?

IMO it is easier to follow in one separate folder. If you want to check all our presets, you could check one place. In the other case, you would need to search all the components to see if there is a preset or not for it. We should be deprecating presets soon enough, but in the meantime, I vote for having a separate presets folder, and a separate one in tests as well to match.

PD: I had the preset folder inside the account folder, but I changed that to match the previous paragraph.

I think that's fair. Sounds good to me

src/account/interface.cairo

docs/modules/ROOT/pages/api/account.adoc

src/account/account.cairo

src/tests/account/test_account.cairo

Scarb.lock

src/tests/account/test_account.cairo

src/tests/account/test_dual_account.cairo

src/tests/mocks/account_mocks.cairo

src/account/interface.cairo

martriay · 2023-10-28T00:43:22Z

src/account/presets/account.cairo

+///
+/// Openzeppelin's account contract.
+#[starknet::contract]
+mod Account {


Agree, but i'd go with BasicAccount or something, to distinguish from future EthereumAccount, or MultisigAccount presets. I'd never call "preset" a contract btw.

…eat/migrate-account-to-component

ericnordelo · 2023-10-30T13:43:11Z

Moved the preset changes to #803

andrew-fleming

LGTM! I just left a question

andrew-fleming · 2023-10-30T21:58:29Z

src/account/account.cairo

 #[starknet::interface]
 trait IPublicKey<TState> {
-    fn set_public_key(ref self: TState, new_public_key: felt252);
    fn get_public_key(self: @TState) -> felt252;
+    fn set_public_key(ref self: TState, new_public_key: felt252);
 }


Since this is an I trait, should we move it to interface.cairo? I'm not sure if we established rules with what goes in there and what stays local in the file

andrew-fleming · 2023-10-30T23:21:50Z

LGTM! I just left a question

We also have to update the API from our "Embeddable Implementations" discussion. Maybe it's best to just slip that in here?

martriay

LGTM! +1 to Andrew's comments

ericnordelo · 2023-10-31T10:54:08Z

I updated the Account API sections. Let me know your thoughts. When we achieve some consensus around the naming of the section, I will update them all (other existing API pages) before merging the PR.

cc @martriay @andrew-fleming

ericnordelo · 2023-10-31T11:17:57Z

I did also update the component names to add the Component suffix.

andrew-fleming · 2023-10-31T16:01:54Z

Very nice! My question was meant just for the account API, but doing them all also works :)

I'm thinking it might be better, in the API, to change EMBEDDABLE CAMELCASE SUPPORT to EMBEDDABLE CAMELCASE IMPLEMENTATIONS to provide more consistency with the naming convention. Wdyt?

Also, we'll have to update the code block under the component title in the API here e.g.
Account
use openzeppelin::account::Account;

ericnordelo · 2023-10-31T17:40:54Z

I'm thinking it might be better, in the API, to change EMBEDDABLE CAMELCASE SUPPORT to EMBEDDABLE CAMELCASE IMPLEMENTATIONS to provide more consistency with the naming convention. Wdyt?

I think I lean more towards EMBEDDABLE IMPLEMENTATIONS (CAMELCASE), just because I think it looks better visually, lol, but no strong defense, let me know which one you prefer and I will update accordingly.

andrew-fleming · 2023-10-31T17:56:47Z

I think I lean more towards EMBEDDABLE IMPLEMENTATIONS (CAMELCASE), just because I think it looks better visually, lol, but no strong defense, let me know which one you prefer and I will update accordingly.

Sounds good to me!

andrew-fleming

LGTM!

martriay

The component and docs heading renames make everything much more clear to me ❤️

ericnordelo added 6 commits January 21, 2023 00:19

fix: link (OpenZeppelin#545)

331844d

Merge branch 'main' of github.com:OpenZeppelin/cairo-contracts

2e7758e

Merge branch 'main' of github.com:OpenZeppelin/cairo-contracts

77cca2d

feat: update logic

4fc9be5

feat: add mocks

ce91ed0

docs: update

4a90358

ericnordelo requested review from andrew-fleming and martriay October 11, 2023 16:23

ericnordelo commented Oct 11, 2023

View reviewed changes

docs/modules/ROOT/pages/api/account.adoc Show resolved Hide resolved

feat: update docs

7f74150

andrew-fleming reviewed Oct 17, 2023

View reviewed changes

ericnordelo and others added 4 commits October 18, 2023 12:38

Update src/account/account.cairo

bff9179

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

Merge branch 'main' of github.com:OpenZeppelin/cairo-contracts into f…

4415191

…eat/migrate-account-to-component

feat: apply review updates

100c483

Merge branch 'feat/migrate-account-to-component' of github.com:ericno…

b3e2106

…rdelo/cairo-contracts into feat/migrate-account-to-component

ericnordelo requested a review from andrew-fleming October 18, 2023 10:59

ericnordelo added 2 commits October 18, 2023 14:19

Merge branch 'main' of github.com:OpenZeppelin/cairo-contracts into f…

0570e44

…eat/migrate-account-to-component

feat: update overview

52df8b1

andrew-fleming approved these changes Oct 18, 2023

View reviewed changes

martriay reviewed Oct 23, 2023

View reviewed changes

ericnordelo and others added 6 commits October 23, 2023 12:10

Update docs/modules/ROOT/pages/api/account.adoc

9705ce7

Co-authored-by: Martín Triay <martriay@gmail.com>

feat: apply review updates

feeac44

Merge branch 'feat/migrate-account-to-component' of github.com:ericno…

0f64992

…rdelo/cairo-contracts into feat/migrate-account-to-component

Merge branch 'main' of github.com:OpenZeppelin/cairo-contracts into f…

f9ce9eb

…eat/migrate-account-to-component

feat: flatten events

fcdcdf7

feat: add account preset

d12a610

ericnordelo requested review from martriay and andrew-fleming October 25, 2023 11:24

andrew-fleming reviewed Oct 25, 2023

View reviewed changes

martriay reviewed Oct 28, 2023

View reviewed changes

ericnordelo added 4 commits October 30, 2023 14:17

feat: apply review updates

66c3fd7

Merge branch 'main' of github.com:OpenZeppelin/cairo-contracts into f…

daa9d68

…eat/migrate-account-to-component

feat: apply review updates

495ff94

feat: remove preset

472b04c

ericnordelo requested review from martriay and andrew-fleming October 30, 2023 13:43

andrew-fleming approved these changes Oct 30, 2023

View reviewed changes

martriay approved these changes Oct 31, 2023

View reviewed changes

feat: apply review updates

a83b810

feat: rename components

cc8d2f9

ericnordelo added 2 commits October 31, 2023 12:23

feat: update docs

296122f

fix: remove failing macro

2e16eaa

fix: imports in docs

d53d1c9

docs: update section title

265faa7

ericnordelo requested review from martriay and andrew-fleming October 31, 2023 18:40

fix: missing title update

c89e8b6

andrew-fleming approved these changes Oct 31, 2023

View reviewed changes

feat: use get component macro

4eeafa9

martriay approved these changes Nov 1, 2023

View reviewed changes

martriay merged commit 69dc448 into OpenZeppelin:main Nov 1, 2023
1 check passed

ericnordelo deleted the feat/migrate-account-to-component branch November 1, 2023 22:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Account to component #783

Migrate Account to component #783

ericnordelo commented Oct 10, 2023

andrew-fleming left a comment

andrew-fleming left a comment

martriay Oct 23, 2023

ericnordelo Oct 23, 2023

martriay Oct 28, 2023 •

edited

Loading

ericnordelo commented Oct 25, 2023

andrew-fleming left a comment

andrew-fleming Oct 25, 2023

martriay Oct 28, 2023

ericnordelo Oct 30, 2023

martriay Oct 31, 2023 •

edited

Loading

andrew-fleming Oct 25, 2023

andrew-fleming Oct 25, 2023

ericnordelo Oct 30, 2023 •

edited

Loading

andrew-fleming Oct 30, 2023

martriay Oct 28, 2023

ericnordelo commented Oct 30, 2023

andrew-fleming left a comment

andrew-fleming Oct 30, 2023

andrew-fleming commented Oct 30, 2023

martriay left a comment

ericnordelo commented Oct 31, 2023

ericnordelo commented Oct 31, 2023

andrew-fleming commented Oct 31, 2023

ericnordelo commented Oct 31, 2023

andrew-fleming commented Oct 31, 2023

andrew-fleming left a comment

martriay left a comment

		component!(path: account_component, storage: account, event: AccountEvent);
		component!(path: src5_component, storage: src5, event: SRC5Event);

Migrate Account to component #783

Migrate Account to component #783

Conversation

ericnordelo commented Oct 10, 2023

PR Checklist

andrew-fleming 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

martriay Oct 28, 2023 • edited Loading

Choose a reason for hiding this comment

ericnordelo commented Oct 25, 2023

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

martriay Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericnordelo Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericnordelo commented Oct 30, 2023

andrew-fleming left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew-fleming commented Oct 30, 2023

martriay left a comment

Choose a reason for hiding this comment

ericnordelo commented Oct 31, 2023

ericnordelo commented Oct 31, 2023

andrew-fleming commented Oct 31, 2023

ericnordelo commented Oct 31, 2023

andrew-fleming commented Oct 31, 2023

andrew-fleming left a comment

Choose a reason for hiding this comment

martriay left a comment

Choose a reason for hiding this comment

martriay Oct 28, 2023 •

edited

Loading

martriay Oct 31, 2023 •

edited

Loading

ericnordelo Oct 30, 2023 •

edited

Loading