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

Migrate account #616

Closed
wants to merge 65 commits into from
Closed
Show file tree
Hide file tree
Changes from 51 commits
Commits
Show all changes
65 commits
Select commit Hold shift + click to select a range
c6996fa
Merge branch 'cairo-1' into erc165
martriay Mar 8, 2023
387637e
Merge branch 'erc165' into account
martriay Mar 8, 2023
90a7a4b
Merge branch 'cairo-1' into account
martriay Mar 8, 2023
f66a988
Merge branch 'cairo-1' into erc165
martriay Mar 8, 2023
e2d7248
continue account implementation
martriay Mar 8, 2023
52019ac
add missing account interface functions
martriay Mar 8, 2023
50b96bf
tidy up module
martriay Mar 8, 2023
aae6451
fix validate
martriay Mar 8, 2023
4974643
Merge branch 'cairo-1' into account
martriay Mar 8, 2023
c485e60
bump cairo + account changes
martriay Mar 9, 2023
921e61f
fix __execute__, add serde, rename felt>felt252
martriay Mar 15, 2023
fc2d995
tidy up code
martriay Mar 15, 2023
f756504
add account tests
martriay Mar 21, 2023
ec321c0
complete account implementation
martriay Mar 24, 2023
27b0f7e
Apply suggestions from code review
martriay Mar 24, 2023
f2a43f5
apply review suggestions
martriay Mar 25, 2023
7e5b91b
Merge branch 'cairo-1' into erc165
martriay Mar 25, 2023
cbfb2dd
remove unused import
martriay Mar 25, 2023
2b9cfd7
merge erc165 changes
martriay Mar 25, 2023
2111a20
clarify __execute__ guard
martriay Mar 25, 2023
bd5979e
add account tests
martriay Mar 25, 2023
995fadd
add internals
martriay Mar 25, 2023
c2d4721
tidy up
martriay Mar 25, 2023
1b9b311
Merge branch 'cairo-1' into erc165
martriay Mar 28, 2023
52357cc
add internal macro
martriay Mar 28, 2023
c615ca3
Merge branch 'erc165' into account
martriay Mar 28, 2023
cee8f05
add internal macro
martriay Mar 28, 2023
d2ddedf
add deregister
martriay Mar 31, 2023
a65e1ef
Merge branch 'erc165' into account
martriay Mar 31, 2023
2e859a4
update erc165
martriay Mar 31, 2023
d2219b6
Merge branch 'cairo-1' into erc165
martriay Mar 31, 2023
fa14e09
Merge branch 'erc165' into account
martriay Mar 31, 2023
c648eff
feat: refactor account and add validate_transaction test
ericnordelo May 4, 2023
a10b46b
feat: work on span serde
ericnordelo May 4, 2023
07225fb
feat: add test for validate
ericnordelo May 4, 2023
c6e9d95
refactor: remove unnecessary imports
ericnordelo May 4, 2023
667bc0a
refactor: tests
ericnordelo May 4, 2023
74c5452
feat: implement test_execute
ericnordelo May 5, 2023
c37f08a
Update src/openzeppelin/tests/test_account.cairo
ericnordelo May 8, 2023
3ea12ff
Update src/openzeppelin/tests/test_account.cairo
ericnordelo May 8, 2023
7de6a2e
feat: update from review
ericnordelo May 8, 2023
025dd97
Merge branch 'feat/test-account' of github.com:ericnordelo/cairo-cont…
ericnordelo May 8, 2023
a3b0919
Merge branch 'cairo-1' of github.com:OpenZeppelin/cairo-contracts int…
ericnordelo May 8, 2023
a034143
feat: format files
ericnordelo May 8, 2023
c24b3a5
feat: SpanSerde compiling version
ericnordelo May 9, 2023
43b19b0
feat: update tests and interface
ericnordelo May 9, 2023
e0664f1
feat: format file
ericnordelo May 9, 2023
e02e348
feat: add test for multicall
ericnordelo May 10, 2023
ec8620e
feat: add more tests
ericnordelo May 10, 2023
e8b19e6
test: remove unused function
ericnordelo May 10, 2023
3d8a6a5
refactor: update ERC20 interface and module
ericnordelo May 10, 2023
649d36b
feat: add impl again
ericnordelo May 11, 2023
f51c1d7
refactor: apply review comments
ericnordelo May 12, 2023
4db1f2f
test: execute out of gas
ericnordelo May 15, 2023
bce4724
refactor: is valid signature because of lack of short circuit condici…
ericnordelo May 15, 2023
c7e18db
refactor: apply updates from reviews
ericnordelo May 15, 2023
b9c3051
Merge branch 'cairo-1' of github.com:OpenZeppelin/cairo-contracts int…
ericnordelo May 15, 2023
d94c8b1
feat: add SpanSerde implementation
ericnordelo May 15, 2023
04ed19c
refactor: remove span_to_array
ericnordelo May 15, 2023
18f3cb5
Update src/openzeppelin/tests/test_account.cairo
ericnordelo May 16, 2023
27280f6
Update src/openzeppelin/account.cairo
ericnordelo May 16, 2023
00b4110
test: update from review
ericnordelo May 17, 2023
98bf7ef
feat: splitting account interface and abi
ericnordelo May 17, 2023
630e300
Merge branch 'feat/test-account' of github.com:ericnordelo/cairo-cont…
ericnordelo May 17, 2023
164b962
feat: add new account files
ericnordelo May 17, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
263 changes: 84 additions & 179 deletions Cargo.lock

Large diffs are not rendered by default.

13 changes: 7 additions & 6 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,8 @@
members = [
"cairo/crates/cairo-lang-casm",
"cairo/crates/cairo-lang-compiler",
"cairo/crates/cairo-lang-utils",
"cairo/crates/cairo-lang-debug",
"cairo/crates/cairo-lang-defs",
"cairo/crates/cairo-lang-proc-macros",
"cairo/crates/cairo-lang-diagnostics",
"cairo/crates/cairo-lang-eq-solver",
"cairo/crates/cairo-lang-filesystem",
Expand All @@ -15,23 +13,25 @@ members = [
"cairo/crates/cairo-lang-lowering",
"cairo/crates/cairo-lang-parser",
"cairo/crates/cairo-lang-plugins",
"cairo/crates/cairo-lang-proc-macros",
"cairo/crates/cairo-lang-project",
"cairo/crates/cairo-lang-runner",
"cairo/crates/cairo-lang-semantic",
"cairo/crates/cairo-lang-sierra",
"cairo/crates/cairo-lang-sierra-ap-change",
"cairo/crates/cairo-lang-sierra-gas",
"cairo/crates/cairo-lang-sierra-generator",
"cairo/crates/cairo-lang-sierra-to-casm",
"cairo/crates/cairo-lang-sierra",
"cairo/crates/cairo-lang-starknet",
"cairo/crates/cairo-lang-syntax-codegen",
"cairo/crates/cairo-lang-syntax",
"cairo/crates/cairo-lang-syntax-codegen",
"cairo/crates/cairo-lang-test-runner",
"cairo/crates/cairo-lang-utils",
"cairo/tests",
]

[workspace.package]
version = "1.0.0-alpha.7"
version = "1.0.0-rc0"
edition = "2021"
repository = "https://github.com/starkware-libs/cairo/"
license = "Apache-2.0"
Expand All @@ -45,7 +45,6 @@ assert_matches = "1.5"
bimap = "0.6.2"
cairo-felt = "0.3.0-rc1"
cairo-vm = "0.3.0-rc1"
chrono = "0.4.23"
clap = { version = "4.0", features = ["derive"] }
colored = "2"
const-fnv1a-hash = "1.1.0"
Expand All @@ -60,6 +59,7 @@ ignore = "0.4.20"
indexmap = { version = "1.9.1", features = ["serde"] }
indoc = "2.0.1"
itertools = "0.10.3"
keccak = "0.1.3"
lalrpop-util = { version = "0.19.9", features = ["lexer"] }
log = "0.4"
lsp = { version = "0.93", package = "lsp-types" }
Expand All @@ -84,6 +84,7 @@ test-case = "2.2.2"
test-case-macros = "2.2.2"
test-log = "0.2.11"
thiserror = "1.0.32"
time = { version = "0.3.20", features = ["formatting", "macros", "local-offset"] }
tokio = { version = "1.18.2", features = ["full", "sync"] }
toml = "0.4.2"
tower-lsp = "0.17.0"
Expand Down
2 changes: 1 addition & 1 deletion cairo
Submodule cairo updated 441 files
201 changes: 201 additions & 0 deletions src/openzeppelin/account.cairo
Original file line number Diff line number Diff line change
@@ -0,0 +1,201 @@
use starknet::ContractAddress;
use openzeppelin::utils::check_gas;
use openzeppelin::utils::span_to_array;
ericnordelo marked this conversation as resolved.
Show resolved Hide resolved

const ERC165_ACCOUNT_ID: u32 = 0xa66bd575_u32;
ericnordelo marked this conversation as resolved.
Show resolved Hide resolved
const ERC1271_VALIDATED: u32 = 0x1626ba7e_u32;

const TRANSACTION_VERSION: felt252 = 1;
// 2**128 + TRANSACTION_VERSION
const QUERY_VERSION: felt252 = 340282366920938463463374607431768211457;

#[derive(Serde, Drop)]
struct Call {
to: ContractAddress,
selector: felt252,
calldata: Array<felt252>
}

#[abi]
trait IAccount {
ericnordelo marked this conversation as resolved.
Show resolved Hide resolved
#[external]
fn __execute__(calls: Array<Call>) -> Array<Array<felt252>>;
#[external]
fn __validate__(calls: Array<Call>) -> felt252;
#[external]
fn __validate_declare__(class_hash: felt252) -> felt252;
#[external]
fn __validate_deploy__(
class_hash: felt252, contract_address_salt: felt252, _public_key: felt252
) -> felt252;
#[external]
fn set_public_key(new_public_key: felt252);
#[view]
fn get_public_key() -> felt252;
#[view]
martriay marked this conversation as resolved.
Show resolved Hide resolved
fn is_valid_signature(message: felt252, signature: Array<felt252>) -> u32;
#[view]
fn supports_interface(interface_id: u32) -> bool;
}

#[account_contract]
mod Account {
use array::SpanTrait;
use array::ArrayTrait;
use box::BoxTrait;
use option::OptionTrait;
use zeroable::Zeroable;
use ecdsa::check_ecdsa_signature;
use serde::ArraySerde;
ericnordelo marked this conversation as resolved.
Show resolved Hide resolved
use starknet::get_tx_info;
use starknet::get_caller_address;
use starknet::get_contract_address;

use super::Call;
use super::ERC165_ACCOUNT_ID;
use super::ERC1271_VALIDATED;
use super::TRANSACTION_VERSION;
use super::QUERY_VERSION;
use super::span_to_array;

use openzeppelin::introspection::erc165::ERC165;
use openzeppelin::utils::check_gas;

//
// Storage and Constructor
//

struct Storage {
public_key: felt252
}

#[constructor]
fn constructor(_public_key: felt252) {
ERC165::register_interface(ERC165_ACCOUNT_ID);
ericnordelo marked this conversation as resolved.
Show resolved Hide resolved
martriay marked this conversation as resolved.
Show resolved Hide resolved
public_key::write(_public_key);
}

//
// Externals
//

// TODO: Use Span in the inner Array of the return type
ericnordelo marked this conversation as resolved.
Show resolved Hide resolved
#[external]
fn __execute__(mut calls: Array<Call>) -> Array<Array<felt252>> {
// avoid calls from other contracts
// https://github.com/OpenZeppelin/cairo-contracts/issues/344
ericnordelo marked this conversation as resolved.
Show resolved Hide resolved
let sender = get_caller_address();
assert(sender.is_zero(), 'Account: invalid caller');

// check tx version
let tx_info = get_tx_info().unbox();
let version = tx_info.version;
if version != TRANSACTION_VERSION { // > operator not defined for felt252
assert(version == QUERY_VERSION, 'Account: invalid tx version');
}

_execute_calls(calls)
}

#[external]
fn __validate__(mut calls: Array<Call>) -> felt252 {
_validate_transaction()
}

#[external]
fn __validate_declare__(class_hash: felt252) -> felt252 {
_validate_transaction()
}

#[external]
fn __validate_deploy__(
class_hash: felt252, contract_address_salt: felt252, _public_key: felt252
) -> felt252 {
_validate_transaction()
}

#[external]
fn set_public_key(new_public_key: felt252) {
_assert_only_self();
public_key::write(new_public_key);
}

//
// View
//

#[view]
fn get_public_key() -> felt252 {
public_key::read()
}

#[view]
fn is_valid_signature(message: felt252, signature: Array<felt252>) -> u32 {
if _is_valid_signature(message, signature.span()) {
ERC1271_VALIDATED
} else {
0_u32
}
}

#[view]
fn supports_interface(interface_id: u32) -> bool {
ERC165::supports_interface(interface_id)
}

//
// Internals
//

#[internal]
fn _assert_only_self() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fn _assert_only_self() {
fn assert_only_self() {

now that we have better idea of how extensibility looks and thanks to the #[internal] annotation, i think we should remove the underscore to signal this is part of the module's API

Copy link
Member Author

Choose a reason for hiding this comment

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

remove the underscore to signal this is part of the module's API

The convention is not underscoring internal functions as in Solidity? What do you mean by this? Should we remove the underscore in the rest of the internals as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

according to our naming conventions, we use the underscore to signal a function's usage is discouraged. this is not the case for assert_only_self, which is ok to use. we should probably rework at least the categories (internal, external, public, private) to accommodate under Cairo 1.0

Solidity's conventions didn't apply to Cairo 0.x, less to Cairo 1.0

Copy link
Collaborator

Choose a reason for hiding this comment

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

we should probably rework at least the categories (internal, external, public, private) to accommodate under Cairo 1.0

Agreed^

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it, but I don't think these naming conventions apply anymore for Cairo 1. Should we remove the underscore for _execute_calls as well, being this not a subset of the public API anymore (with the internal keyword) ? Should all the internal functions avoid underscore when possible? Like _mint in ERC20 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

we should probably rework at least the categories (internal, external, public, private) to accommodate under Cairo 1.0

Agreed, but what rules are we applying in the meantime? Why should we remove the underscore for assert_only_self and not for other internal functions?

Copy link
Contributor

Choose a reason for hiding this comment

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

are those internal functions meant to be used by users without special precaution? if so, it can be removed. e.g. assert_only_self is a modifier that can, and it's expected, to be used as is. whereas _mint should be very carefully used since it lacks some invariant checks making it error prone, then we use the underscore. i believe this is addressed in our docs already.

what i think we need to rework is the names of the categories (internal/external/private/public) because there's a new language environment, but i don't see any problem with the concepts behind the naming. any counter examples where the convention doesn't work anymore?

let caller = get_caller_address();
let self = get_contract_address();
assert(self == caller, 'Account: unauthorized');
}

#[internal]
fn _validate_transaction() -> felt252 {
let tx_info = get_tx_info().unbox();
let tx_hash = tx_info.transaction_hash;
let signature = tx_info.signature;
assert(_is_valid_signature(tx_hash, signature), 'Account: invalid signature');
starknet::VALIDATED
}

#[internal]
fn _is_valid_signature(message: felt252, signature: Span<felt252>) -> bool {
let valid_length = signature.len() == 2_u32;

valid_length
& check_ecdsa_signature(
message, public_key::read(), *signature.at(0_u32), *signature.at(1_u32)
)
}

#[internal]
fn _execute_calls(mut calls: Array<Call>) -> Array<Array<felt252>> {
let mut res = ArrayTrait::new();
loop {
match calls.pop_front() {
Option::Some(call) => {
let _res = _execute_single_call(call);
res.append(_res);
},
Option::None(_) => {
break ();
},
}
check_gas();
ericnordelo marked this conversation as resolved.
Show resolved Hide resolved
};
res
}

#[internal]
fn _execute_single_call(call: Call) -> Array<felt252> {
let Call{to, selector, calldata } = call;

let res = starknet::call_contract_syscall(to, selector, calldata.span()).unwrap_syscall();
span_to_array(res)
ericnordelo marked this conversation as resolved.
Show resolved Hide resolved
}
}
1 change: 1 addition & 0 deletions src/openzeppelin/lib.cairo
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
mod introspection;
mod security;
mod account;
mod token;
mod tests;
mod utils;
1 change: 1 addition & 0 deletions src/openzeppelin/tests.cairo
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
mod test_erc165;
mod test_account;
mod test_erc20;
mod test_pausable;
mod test_initializable;
Expand Down
Loading