-
Notifications
You must be signed in to change notification settings - Fork 356
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
Add macros package and the with_components macro #1282
base: main
Are you sure you want to change the base?
Add macros package and the with_components macro #1282
Conversation
…eat/add-macros-package
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.
Fantastic work on this @ericnordelo! I left some comments
Also, since so much of the component boilerplate is abstracted away, I don't think it's super obvious what the storage entry is for a given component. It'll be documented, for sure, but would it make sense just to simplify it further and use the same capitalized name as the whitelisted component i.e.
#[with_components(ERC20, Ownable)]
#[starknet::contract]
pub mod MyToken {
(...)
#[constructor]
fn constructor(ref self: ContractState, owner: ContractAddress) {
self.Ownable.initializer(owner);
self.ERC20.initializer("MyToken", "MTK");
}
}
This gives a point of reference inside the contract as opposed to attempting, for example, reentrancy_guard
or reentrancyguard
...ts/openzeppelin_macros__tests__test_with_components__with_access_control_no_initializer.snap
Outdated
Show resolved
Hide resolved
...snapshots/openzeppelin_macros__tests__test_with_components__with_account_no_initializer.snap
Show resolved
Hide resolved
.../openzeppelin_macros__tests__test_with_components__with_erc1155_receiver_no_initializer.snap
Show resolved
Hide resolved
"AccessControl" => Some(ComponentInfo { | ||
name: "AccessControlComponent".to_string(), | ||
path: "openzeppelin_access::accesscontrol::AccessControlComponent".to_string(), | ||
storage: "accesscontrol".to_string(), |
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.
Same here. I know the lib isn't entirely consistent (might be worth addressing in a separate issue), but I think it could get weird when we have timelock_controller
vs accesscontrol
and reentrancyguard
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.
I will refactor them here (for macros). We may update presets and mocks in a separate issue later.
Co-authored-by: Andrew Fleming <[email protected]>
Co-authored-by: Andrew Fleming <[email protected]>
Co-authored-by: Andrew Fleming <[email protected]>
Co-authored-by: Andrew Fleming <[email protected]>
Co-authored-by: Andrew Fleming <[email protected]>
Since developers are already using snake case for storage members, I vote we stay with snake case, but split the words accordingly. |
This is true...no strong pushback against keeping snake case, I just think reusing the name could be a bit more intuitive. If we wanted to make a change like this, it'd be better to do it now with the feature |
Quickstart #1258
NOTE: A summary of the features included in the macro is added below after the example:
Example:
Simplifies this:
Into this:
Summary of features:
IT DOES:
use openzeppelin_access::ownable::OwnableComponent;
)component!(path: OwnableComponent, storage: ownable, event: OwnableEvent);
).impl OwnableInternalImpl = OwnableComponent::InternalImpl<ContractState>;
)ownable: OwnableComponent::Storage,
)IT DOES NOT:
Things to be considered still:
openzeppelin_access
vsopenzeppelin::access
in use clauses (configurable notation?)Some may be addressed in different PRs/issues.
NOTE: I have plans to include a
with_component
macro which will accept only a single component but more configuration, like replacing the storage entry. People then may use both macros together (with_components
andwith_component
)PR Checklist