Add EthAccount #853

ericnordelo · 2023-12-12T12:19:36Z

Fixes #573

PR Checklist

Tests
Documentation
Added entry to CHANGELOG.md
Tried the feature on a public network

…eat/migrate-eth-account-#573

andrew-fleming

Great start, Eric! I left some feedback.

I also have some doubts on the EthAccount dual dispatcher. The v0 eth account pubkey used the felt252 eth address; whereas, this implementation uses the secp256k1point eth pubkey. Backward compatibility will be an issue. Do you agree?

Also, this review is just for the code. I didn't want to wait any longer, so I haven't looked at the docs yet

src/account/eth_account/eth_account.cairo

src/account/eth_account/interface.cairo

src/tests/mocks/eth_account_mocks.cairo

src/presets/eth_account.cairo

src/account/utils.cairo

src/account/utils/secp256k1.cairo

Co-authored-by: Andrew Fleming <[email protected]>

…o/cairo-contracts into feat/migrate-eth-account-#573

ericnordelo · 2023-12-21T14:24:53Z

I also have some doubts on the EthAccount dual dispatcher. The v0 eth account pubkey used the felt252 eth address; whereas, this implementation uses the secp256k1point eth pubkey. Backward compatibility will be an issue. Do you agree?

I don't think we need to be BC with the cairo 0 version, but I'm wondering if we should add the camelCase interfaces for this module at all. A reason to keep it is that some contracts and modules can still be using isValidSignature, and this could be compatible by just passing a different signature (secp256k1 compatible). cc @martriay

andrew-fleming

Nice work on the docs! I know this is still a WIP, but I left a few comments and questions

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

martriay

Great progress! Left some partial comments, still missing a few modules.

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

martriay · 2023-12-23T04:14:46Z

docs/modules/ROOT/pages/utils/_class_hashes.adoc

@@ -3,6 +3,7 @@

 // Class Hashes
 :account-class-hash: 0x016c6081eb34ad1e0c5513234ed0c025b3c7f305902d291bad534cd6474c85bc
+:eth-account-upgradeable-class-hash: TBD


comment to remind us to complete this once we have it

src/account/eth_account/interface.cairo

src/account/eth_account/eth_account.cairo

src/account/utils/secp256k1.cairo

src/account/utils/signature.cairo

martriay · 2023-12-29T01:25:16Z

src/account/eth_account/eth_account.cairo

+    #[derive(Drop, starknet::Event)]
+    struct OwnerAdded {
+        #[key]
+        new_owner_guid: felt252
+    }
+
+    #[derive(Drop, starknet::Event)]
+    struct OwnerRemoved {
+        #[key]
+        removed_owner_guid: felt252
+    }
+
+    mod Errors {
+        const INVALID_CALLER: felt252 = 'EthAccount: invalid caller';
+        const INVALID_SIGNATURE: felt252 = 'EthAccount: invalid signature';
+        const INVALID_TX_VERSION: felt252 = 'EthAccount: invalid tx version';
+        const UNAUTHORIZED: felt252 = 'EthAccount: unauthorized';
+    }


i wonder if this can be abstracted out so it's easily reused in both modules somehow

Since we are prefixing messages with the module name, I don't think so. Do you have something in mind? We could implement traits with different implementations in scope, but that would be overkilling the issue imo.

I was thinking more about normalizing the errors as Account: ... because I still feel like treating Account and EthAccount as roughly the same component, the separation wasn't ideal but a workaround. No strong opinions, we can leave it like this.

martriay · 2023-12-29T01:28:17Z

src/account/eth_account/eth_account.cairo

+        ///
+        /// Requirements:
+        ///
+        /// - The transaction version must be `TRANSACTION_VERSION` for actual transactions.


this should be updated after #858

ericnordelo · 2024-01-03T11:30:23Z

@andrew-fleming I've pushed the updates regarding error messages, after we confirm we are ok with it, I will update the rest in the other PR. The rules I'm following:

Use assertion macros instead of inline functions (assert! and assert_eq!). These allow us to pass ByteArrays instead of short strings.
Use assert_eq! over assert! when applicable.
Avoid adding messages to assertions as much as possible to avoid redundancy. There are cases where messages are helpful, but sometimes they are redundant. for example, assert!(big_point_1 != big_point_2); shouldn't need a message while assert_eq!(parity, true, "Parity should be odd"); add some clarity.
When the message is helpful, I'm following these short rules by basically using "X should be Y" or "X should have Y" or just "Should be/have Y", etc...

Let me know what you think.

andrew-fleming

I just left a small comment and a question on the same minor thing. Overall, the error messages look good though. Nice work!

...after we confirm we are ok with it, I will update the rest in the other PR

Good call 👍

src/tests/account/test_eth_account.cairo

src/tests/account/test_dual_eth_account.cairo

src/tests/account/test_eth_account.cairo

martriay

Really great job! It looks very good to me. I think we're ready to test this onchain.

src/tests/presets/test_eth_account.cairo

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

Co-authored-by: Martín Triay <[email protected]>

…o/cairo-contracts into feat/migrate-eth-account-#573

…eat/migrate-eth-account-#573

martriay · 2024-01-29T02:43:34Z

We can fix the conflicts and merge.

…eat/migrate-eth-account-#573

src/account/account/account.cairo

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

martriay

fix conflicts and good to merge!

…eat/migrate-eth-account-#573

ericnordelo added 3 commits December 12, 2023 13:18

feat: add eth_account main logic

cd13579

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

19ce07d

…eat/migrate-eth-account-#573

feat: add tests

463d663

ericnordelo marked this pull request as ready for review December 15, 2023 15:36

fix: workflow

6ce5a08

ericnordelo requested review from martriay and andrew-fleming December 15, 2023 15:40

ericnordelo added 6 commits December 19, 2023 16:25

feat: change EthPublicKey to Secp256k1Point

e2ee218

feat: add more tests

3fdaf15

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

6cdbac9

…eat/migrate-eth-account-#573

feat: add entry to CHANGELOG

16427fc

feat: add tests for preset

135e8f1

docs: add API Reference entries

f9c811a

andrew-fleming reviewed Dec 21, 2023

View reviewed changes

ericnordelo and others added 3 commits December 21, 2023 14:28

Update src/account/eth_account/eth_account.cairo

b883c6b

Co-authored-by: Andrew Fleming <[email protected]>

feat: apply review updates

3c396de

Merge branch 'feat/migrate-eth-account-#573' of github.com:ericnordel…

ba60238

…o/cairo-contracts into feat/migrate-eth-account-#573

refactor: test

34e3010

andrew-fleming reviewed Dec 27, 2023

View reviewed changes

feat: apply review updates

b40970b

martriay reviewed Dec 29, 2023

View reviewed changes

feat: update error messages

e960a4a

andrew-fleming reviewed Jan 4, 2024

View reviewed changes

src/tests/account/test_eth_account.cairo Outdated Show resolved Hide resolved

src/tests/account/test_dual_eth_account.cairo Outdated Show resolved Hide resolved

feat: apply review updates

79c64a2

martriay reviewed Jan 17, 2024

View reviewed changes

src/tests/account/test_dual_eth_account.cairo Show resolved Hide resolved

src/tests/account/test_eth_account.cairo Show resolved Hide resolved

martriay reviewed Jan 17, 2024

View reviewed changes

src/tests/presets/test_eth_account.cairo Show resolved Hide resolved

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

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

ericnordelo and others added 2 commits January 17, 2024 11:38

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

facf8ab

Co-authored-by: Martín Triay <[email protected]>

feat: add tests for upgrade

ebc2e9d

ericnordelo added 5 commits January 17, 2024 12:55

feat: add tests for signature

1ef85b9

Merge branch 'feat/migrate-eth-account-#573' of github.com:ericnordel…

9c99ffb

…o/cairo-contracts into feat/migrate-eth-account-#573

refactor: drop X.Y.Z in favor of last released version

a723d53

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

f17f151

…eat/migrate-eth-account-#573

feat: updated class hashes

ef4128c

ericnordelo added 3 commits January 29, 2024 12:14

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

de0c40c

…eat/migrate-eth-account-#573

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

73144d5

…eat/migrate-eth-account-#573

fix: casm target

42abd67

ericnordelo requested a review from martriay January 29, 2024 11:34

martriay approved these changes Jan 29, 2024

View reviewed changes

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

martriay reviewed Jan 29, 2024

View reviewed changes

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

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

ericnordelo added 2 commits January 30, 2024 12:19

fix: versions

26651b3

refactor: account directory

97151fd

martriay approved these changes Jan 31, 2024

View reviewed changes

ericnordelo added 3 commits January 31, 2024 11:51

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

c9d50e4

…eat/migrate-eth-account-#573

fix: comments

8a3939b

fix: class hashes

0f9b41c

ericnordelo merged commit 6d26b84 into OpenZeppelin:main Jan 31, 2024
5 checks passed

ericnordelo deleted the feat/migrate-eth-account-#573 branch January 31, 2024 11:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add EthAccount #853

Add EthAccount #853

ericnordelo commented Dec 12, 2023 •

edited

Loading

andrew-fleming left a comment •

edited

Loading

ericnordelo commented Dec 21, 2023

andrew-fleming left a comment

martriay left a comment

martriay Dec 23, 2023

martriay Dec 29, 2023

ericnordelo Jan 17, 2024

martriay Jan 17, 2024

martriay Dec 29, 2023

ericnordelo commented Jan 3, 2024 •

edited

Loading

andrew-fleming left a comment

martriay left a comment

martriay commented Jan 29, 2024 •

edited

Loading

martriay left a comment

Add EthAccount #853

Add EthAccount #853

Conversation

ericnordelo commented Dec 12, 2023 • edited Loading

PR Checklist

andrew-fleming left a comment • edited Loading

Choose a reason for hiding this comment

ericnordelo commented Dec 21, 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 Dec 23, 2023

Choose a reason for hiding this comment

martriay Dec 29, 2023

Choose a reason for hiding this comment

ericnordelo Jan 17, 2024

Choose a reason for hiding this comment

martriay Jan 17, 2024

Choose a reason for hiding this comment

martriay Dec 29, 2023

Choose a reason for hiding this comment

ericnordelo commented Jan 3, 2024 • edited Loading

andrew-fleming left a comment

Choose a reason for hiding this comment

martriay left a comment

Choose a reason for hiding this comment

martriay commented Jan 29, 2024 • edited Loading

martriay left a comment

Choose a reason for hiding this comment

ericnordelo commented Dec 12, 2023 •

edited

Loading

andrew-fleming left a comment •

edited

Loading

ericnordelo commented Jan 3, 2024 •

edited

Loading

martriay commented Jan 29, 2024 •

edited

Loading