-
Notifications
You must be signed in to change notification settings - Fork 359
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 Ownable to component #768
Changes from 118 commits
331844d
b9ec20b
bb40124
a0c3157
3a6e20d
96431de
7b38706
2fa00dd
f0036e7
8d781a8
ae37b1c
da57a91
fd42fdd
1cb1252
f0330cd
abc737a
4ca2620
8dd8ed3
091a29c
1c866f2
3405063
8fe3c82
ad928bc
d883eb5
543501b
2ecba91
6c3978b
60365c8
be1e9df
c20fd02
bce2506
35ede09
e55889a
9d5079c
ae89fa5
a54e98c
91dcad3
f48da2c
d671d4c
e86bcd2
42a160f
598c232
b2624ee
4a4bca9
0962051
a1950ee
c29b45c
e48a682
5f86f65
897facb
372de37
d86eb73
62688e7
ba69328
8534ec6
bc90bd5
c8fa9f9
3a8e3e7
6a1f729
bbcb875
5a27baa
ba29133
b44f7dc
33dbd56
65d03ae
a863859
c4590b9
f18fcdb
dc4a233
27514b1
8e10821
a00a9d3
0b40485
4e64434
79fe2db
177ad63
495ed8a
c28c758
f9c30da
457b98b
544ddac
4473243
e95def8
77089c1
3e08fa6
8307644
cece32b
32f9974
b40c35b
fbdd759
05429e4
adac09f
90be39f
fcdd6d1
7373daa
cbb5bf5
4e388f5
a1b559c
74eb4e8
420f320
4f973b6
2f4b1f0
b425e2c
354a2cd
95706b1
d2c538b
83c1203
2ae8139
fb514e1
25fcab8
54396a6
3fc22bc
4f24b82
e112ba1
95f2f76
9d8b0aa
d13b688
c823416
5dd422e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,15 @@ | ||
// SPDX-License-Identifier: MIT | ||
// OpenZeppelin Contracts for Cairo v0.7.0 (access/ownable/ownable.cairo) | ||
|
||
#[starknet::contract] | ||
/// # Ownable Component | ||
/// | ||
/// The Ownable component provides a basic access control mechanism, where | ||
/// there is an account (an owner) that can be granted exclusive access to | ||
/// specific functions. | ||
/// | ||
/// The initial owner can be set by using the `initializer` function in | ||
/// construction time. This can later be changed with `transfer_ownership`. | ||
#[starknet::component] | ||
mod Ownable { | ||
use openzeppelin::access::ownable::interface; | ||
use starknet::ContractAddress; | ||
|
@@ -30,20 +38,72 @@ mod Ownable { | |
const ZERO_ADDRESS_OWNER: felt252 = 'New owner is the zero address'; | ||
} | ||
|
||
#[embeddable_as(OwnableImpl)] | ||
impl Ownable< | ||
TContractState, +HasComponent<TContractState> | ||
> of interface::IOwnable<ComponentState<TContractState>> { | ||
/// Returns the address of the current owner. | ||
fn owner(self: @ComponentState<TContractState>) -> ContractAddress { | ||
self.Ownable_owner.read() | ||
} | ||
|
||
/// Transfers ownership of the contract to a new address. | ||
fn transfer_ownership( | ||
ref self: ComponentState<TContractState>, new_owner: ContractAddress | ||
) { | ||
assert(!new_owner.is_zero(), Errors::ZERO_ADDRESS_OWNER); | ||
self.assert_only_owner(); | ||
self._transfer_ownership(new_owner); | ||
} | ||
|
||
/// Leaves the contract without owner. It will not be possible to call `assert_only_owner` | ||
/// functions anymore. Can only be called by the current owner. | ||
fn renounce_ownership(ref self: ComponentState<TContractState>) { | ||
self.assert_only_owner(); | ||
self._transfer_ownership(Zeroable::zero()); | ||
} | ||
} | ||
|
||
/// Adds camelCase support for `IOwnable`. | ||
#[embeddable_as(OwnableCamelOnlyImpl)] | ||
impl OwnableCamelOnly< | ||
TContractState, +HasComponent<TContractState> | ||
> of interface::IOwnableCamelOnly<ComponentState<TContractState>> { | ||
fn transferOwnership(ref self: ComponentState<TContractState>, newOwner: ContractAddress) { | ||
self.transfer_ownership(newOwner); | ||
} | ||
|
||
fn renounceOwnership(ref self: ComponentState<TContractState>) { | ||
self.renounce_ownership(); | ||
} | ||
ericnordelo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
Comment on lines
+68
to
+79
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking of how users will use the component, IMO it'd be a nice option to also have a single
Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see your point, but the issue I have with it is that I think users should have the ability to opt-in/out of some impl for components, and having a single big ABI enforces every function by default. Examples of components that I think should have multiple external impl are: ERC721: the ERC721Metadata impl should be optional, even when is widely used. The standard doesn't require this, and users should be able to not use it IMO. The ERC20 example as well should have the metadata part as optional, but also the non-standard impl IMO. I think I lean towards letting multiple impl, even when I feel (agree) this could be improved somehow. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Oh, for sure! I was thinking more along the lines of having the implementations that we have now and also one big ABI impl with everything. This gives the user the option to either choose specific impls or use the whole component. I imagine this being quite beneficial for the account and erc721 components The issue I see, though, is we'd have to rewrite all the methods under the ABI impl or create some sort of multi-impl embeddable mechanism There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I lean towards modularity here, having ERC721, ERC721Metadata, and non-standard (with a more descriptive name, not generic) impls separately and required explicitly in the contract. Having an extra impl with the full ABI seems like an antipattern, because it would be harder to follow IMO. |
||
|
||
#[generate_trait] | ||
impl InternalImpl of InternalTrait { | ||
fn initializer(ref self: ContractState, owner: ContractAddress) { | ||
impl InternalImpl< | ||
TContractState, +HasComponent<TContractState> | ||
> of InternalTrait<TContractState> { | ||
/// Sets the contract's initial owner. | ||
/// | ||
/// This function should be called at construction time. | ||
fn initializer(ref self: ComponentState<TContractState>, owner: ContractAddress) { | ||
self._transfer_ownership(owner); | ||
} | ||
|
||
fn assert_only_owner(self: @ContractState) { | ||
/// Panics if called by any account other than the owner. Use this | ||
/// to restrict access to certain functions to the owner. | ||
fn assert_only_owner(self: @ComponentState<TContractState>) { | ||
let owner: ContractAddress = self.Ownable_owner.read(); | ||
let caller: ContractAddress = get_caller_address(); | ||
assert(!caller.is_zero(), Errors::ZERO_ADDRESS_CALLER); | ||
assert(caller == owner, Errors::NOT_OWNER); | ||
} | ||
|
||
fn _transfer_ownership(ref self: ContractState, new_owner: ContractAddress) { | ||
/// Transfers ownership of the contract to a new address. | ||
/// | ||
/// Internal function without access restriction. | ||
fn _transfer_ownership( | ||
ref self: ComponentState<TContractState>, new_owner: ContractAddress | ||
) { | ||
let previous_owner: ContractAddress = self.Ownable_owner.read(); | ||
self.Ownable_owner.write(new_owner); | ||
self | ||
|
@@ -52,33 +112,4 @@ mod Ownable { | |
); | ||
} | ||
} | ||
|
||
#[external(v0)] | ||
impl OwnableImpl of interface::IOwnable<ContractState> { | ||
fn owner(self: @ContractState) -> ContractAddress { | ||
self.Ownable_owner.read() | ||
} | ||
|
||
fn transfer_ownership(ref self: ContractState, new_owner: ContractAddress) { | ||
assert(!new_owner.is_zero(), Errors::ZERO_ADDRESS_OWNER); | ||
self.assert_only_owner(); | ||
self._transfer_ownership(new_owner); | ||
} | ||
|
||
fn renounce_ownership(ref self: ContractState) { | ||
self.assert_only_owner(); | ||
self._transfer_ownership(Zeroable::zero()); | ||
} | ||
} | ||
|
||
#[external(v0)] | ||
impl OwnableCamelOnlyImpl of interface::IOwnableCamelOnly<ContractState> { | ||
fn transferOwnership(ref self: ContractState, newOwner: ContractAddress) { | ||
OwnableImpl::transfer_ownership(ref self, newOwner); | ||
} | ||
|
||
fn renounceOwnership(ref self: ContractState) { | ||
OwnableImpl::renounce_ownership(ref self); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better!