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.

Sign up for GitHub

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

Jump to bottom

Replace ERC-165 with SRC-5 (Update Interface Ids) #637

Merged
merged 10 commits into from Jul 2, 2023
Merged

Replace ERC-165 with SRC-5 (Update Interface Ids) #637

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9d1b405
feat: add src5 module
ericnordelo Jun 9, 2023
d9fc292
refactor: interface ids and account standard interface
ericnordelo Jun 9, 2023
ff67ce7
refactor: naming convention
ericnordelo Jun 9, 2023
c2f5589
refactor: separate interfaces
ericnordelo Jun 13, 2023
b4670b4
feat: apply last SNIP update
ericnordelo Jun 20, 2023
4218698
fix: account interface signature
ericnordelo Jun 23, 2023
66620ff
feat: apply updates from review
ericnordelo Jun 26, 2023
f53efd4
fix: accesscontrol interface id
ericnordelo Jun 26, 2023
68b4e97
Update src/openzeppelin/account/account.cairo
ericnordelo Jun 28, 2023
51eddfe
feat: apply update from review
ericnordelo Jun 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/openzeppelin/access/accesscontrol.cairo
View file
Open in desktop
Expand Up @@ -2,7 +2,7 @@ use starknet::ContractAddress;

const DEFAULT_ADMIN_ROLE: felt252 = 0;
const IACCESSCONTROL_ID: felt252 =
0x262988e98e77072d75a776162d7e30e5fe024a71909613481916261961df736;
0x23700be02858dbe2ac4dc9c9f66d0b6b0ed81ec7f970ca6844500a56ff61751;

#[abi]
trait IAccessControl {
Expand Down
7 changes: 2 additions & 5 deletions src/openzeppelin/account/account.cairo
View file
Open in desktop
Expand Up @@ -49,10 +49,8 @@ mod Account {

use openzeppelin::account::interface::IAccount;
use openzeppelin::account::interface::IACCOUNT_ID;
use openzeppelin::account::interface::IERC1271_VALIDATED;
use openzeppelin::account::interface::ERC1271_VALIDATED;
use openzeppelin::introspection::src5::SRC5;
use openzeppelin::introspection::src5::ISRC5;
use openzeppelin::introspection::src5::ISRC5_ID;

use super::Call;
use super::QUERY_VERSION;
Expand Down Expand Up @@ -95,7 +93,7 @@ mod Account {

fn is_valid_signature(message: felt252, signature: Array<felt252>) -> u32 {
if _is_valid_signature(message, signature.span()) {
IERC1271_VALIDATED
ERC1271_VALIDATED
} else {
0
}
Expand Down Expand Up @@ -164,7 +162,6 @@ mod Account {
#[internal]
fn initializer(_public_key: felt252) {
SRC5::register_interface(IACCOUNT_ID);
SRC5::register_interface(ISRC5_ID);
public_key::write(_public_key);
}

Expand Down
2 changes: 1 addition & 1 deletion src/openzeppelin/account/interface.cairo
View file
Open in desktop
Expand Up @@ -3,7 +3,7 @@ use array::SpanTrait;
use starknet::ContractAddress;

const IACCOUNT_ID: felt252 = 0x36c738c1c375b993078fe6b517d477e5a3c9b104e40c04662c4bdd3e2f5fa4a;
const IERC1271_VALIDATED: u32 = 0x1626ba7e_u32;
const ERC1271_VALIDATED: u32 = 0x1626ba7e_u32;

#[derive(Serde, Drop)]
struct Call {
Expand Down
2 changes: 1 addition & 1 deletion src/openzeppelin/introspection/src5.cairo
View file
Open in desktop
Expand Up @@ -34,7 +34,7 @@ mod SRC5 {

#[internal]
fn deregister_interface(interface_id: felt252) {
assert(interface_id != src5::ISRC5_ID, 'Invalid id');
assert(interface_id != src5::ISRC5_ID, 'SRC5: invalid id');
supported_interfaces::write(interface_id, false);
}
}
4 changes: 2 additions & 2 deletions src/openzeppelin/tests/test_account.cairo
View file
Open in desktop
Expand Up @@ -10,7 +10,7 @@ use openzeppelin::account::Account;
use openzeppelin::account::AccountABIDispatcher;
use openzeppelin::account::AccountABIDispatcherTrait;
use openzeppelin::account::interface::Call;
use openzeppelin::account::interface::IERC1271_VALIDATED;
use openzeppelin::account::interface::ERC1271_VALIDATED;
use openzeppelin::account::interface::IACCOUNT_ID;
use openzeppelin::account::QUERY_VERSION;
use openzeppelin::account::TRANSACTION_VERSION;
Expand Down Expand Up @@ -127,7 +127,7 @@ fn test_is_valid_signature() {
Account::set_public_key(data.public_key);

let is_valid = Account::is_valid_signature(message, good_signature);
assert(is_valid == IERC1271_VALIDATED, 'Should accept valid signature');
assert(is_valid == ERC1271_VALIDATED, 'Should accept valid signature');

let is_valid = Account::is_valid_signature(message, bad_signature);
assert(is_valid == 0, 'Should reject invalid signature');
Expand Down
2 changes: 1 addition & 1 deletion src/openzeppelin/tests/test_src5.cairo
View file
Open in desktop
Expand Up @@ -36,7 +36,7 @@ fn test_deregister_interface() {

#[test]
#[available_gas(2000000)]
#[should_panic(expected: ('Invalid id', ))]
#[should_panic(expected: ('SRC5: invalid id', ))]
fn test_deregister_default_interface() {
SRC5::deregister_interface(ISRC5_ID);
}
2 changes: 1 addition & 1 deletion src/openzeppelin/utils/constants.cairo
View file
Open in desktop
Expand Up @@ -20,7 +20,7 @@ const IERC721_RECEIVER_ID: felt252 =
// AccessControl
// See: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/IAccessControl.sol
const IACCESSCONTROL_ID: felt252 =
0x262988e98e77072d75a776162d7e30e5fe024a71909613481916261961df736;
0x23700be02858dbe2ac4dc9c9f66d0b6b0ed81ec7f970ca6844500a56ff61751;
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think having this constant duplicated here and in access/accesscontrol.cairo is a good idea. let's keep just one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Your suggestion is just for AccessControl? All these constants are duplicated, and not a single one is being used from the constant module. Should I remove just AccessControl, or are you suggesting getting rid of the constants module? (at least current contents)

Copy link
Contributor

Choose a reason for hiding this comment

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

if all constants are already available from their respective modules and no single reference to this module exists (not even in tests) then we should remove it

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no single reference indeed, but maybe it is better to let this for a separate PR? Even when it is a fairly small change, it will delay things. For example, should we add reference comments in the modules? that's not duplicated.

I can open a different issue and submit the PR removing the constants module if you agree.


//
// Roles
Expand Down