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

Apply underscore pattern #993

Merged
merged 12 commits into from
Jun 13, 2024
Merged

Apply underscore pattern #993

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c39732e
feat: apply underscore pattern
ericnordelo May 22, 2024
db22104
fix: mocks
ericnordelo May 22, 2024
9f6d9cc
feat: update PausableComponent
ericnordelo May 27, 2024
10f4723
feat: apply review updates
ericnordelo May 29, 2024
d32e2a4
fix: signatures
ericnordelo May 29, 2024
dc5d004
feat: apply review updates
ericnordelo Jun 3, 2024
93b70fa
Update CHANGELOG.md
ericnordelo Jun 12, 2024
ef06db3
feat: apply review updates
ericnordelo Jun 12, 2024
01e6826
Merge branch 'feat/underscore-internal-functions-#795' of github.com:…
ericnordelo Jun 12, 2024
bcfba01
feat: apply review update
ericnordelo Jun 13, 2024
24ff0d7
Merge branch 'main' of github.com:OpenZeppelin/cairo-contracts into f…
ericnordelo Jun 13, 2024
0a1be85
fix: tests
ericnordelo Jun 13, 2024
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
19 changes: 19 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## Unreleased

### Changed (Breaking)

- Apply underscore pattern to modules (#993)
- AccessControlComponent
- `_change_role_admin` function renamed to `change_role_admin`
ericnordelo marked this conversation as resolved.
Show resolved Hide resolved
- PausableComponent
- `_pause` function renamed to `pause`
- `_unpause` function renamed to `unpause`
- ERC20Component:
- `_mint` function renamed to `mint`
- `_burn` function renamed to `burn`
ericnordelo marked this conversation as resolved.
Show resolved Hide resolved
- ERC721Component:
- `_safe_transfer` function renamed to `safe_transfer`
- `_safe_mint` function renamed to `safe_mint`
- `_burn` function renamed to `burn`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we change _mint to mint since we're doing that with 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.

Similar to _transfer in ERC721, I think we should keep _mint underscored because we don't want users to use them, those are private helpers for the "safe" versions that are not underscored (safe_mint and safe_transfer)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar to _transfer in ERC721, I think we should keep _mint underscored because we don't want users to use them

My impression was that we were taking an agnostic stance regarding x and safe_x because there's some risk in both approaches. They'll still be accessible and this is more of an organizational change, so it's not the end of the world if you prefer it this way. I'm just not a fan of blanket-favoring the safe variants

Copy link
Member Author

Choose a reason for hiding this comment

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

mmm, that's a very good point. Agree.

- ERC1155Component:
- `set_base_uri` function renamed to `_set_base_uri`
- `_update` function renamed to `update`
ericnordelo marked this conversation as resolved.
Show resolved Hide resolved

## 0.13.0 (2024-05-20)

### Added
Expand Down
39 changes: 20 additions & 19 deletions docs/modules/ROOT/pages/api/access.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
:Ownable: xref:OwnableComponent[Ownable]
:src5: https://github.com/starknet-io/SNIPs/blob/main/SNIPS/snip-5.md[SRC5]
:inner-src5: xref:api/introspection.adoc#ISRC5[SRC5 ID]
:_set_role_admin: xref:#AccessControlComponent-_set_role_admin[_set_role_admin]
:set_role_admin: xref:#AccessControlComponent-set_role_admin[set_role_admin]

= Access Control

Expand Down Expand Up @@ -87,9 +87,9 @@ This module includes the internal `assert_only_owner` to restrict a function to

* xref:OwnableComponent-initializer[`++initializer(self, owner)++`]
* xref:OwnableComponent-assert_only_owner[`++assert_only_owner(self)++`]
* xref:OwnableComponent-_accept_ownership[`++_accept_ownership(self)++`]
* xref:OwnableComponent-_propose_owner[`++_propose_owner(self, new_owner)++`]
* xref:OwnableComponent-_transfer_ownership[`++_transfer_ownership(self, new_owner)++`]
* xref:OwnableComponent-_propose_owner[`++_propose_owner(self, new_owner)++`]
* xref:OwnableComponent-_accept_ownership[`++_accept_ownership(self)++`]
--

[.contract-index]
Expand Down Expand Up @@ -221,16 +221,13 @@ Emits an xref:OwnableComponent-OwnershipTransferred[OwnershipTransferred] event.
Panics if called by any account other than the owner.

[.contract-item]
[[OwnableComponent-_accept_ownership]]
==== `[.contract-item-name]#++_accept_ownership++#++(ref self: ContractState)++` [.item-kind]#internal#

Transfers ownership to the pending owner. Resets pending owner to zero address.
Calls xref:OwnableComponent-_transfer_ownership[_transfer_ownership].
[[OwnableComponent-_transfer_ownership]]
==== `[.contract-item-name]#++_transfer_ownership++#++(ref self: ContractState, new_owner: ContractAddress)++` [.item-kind]#internal#

Transfers ownership of the contract to a new account (`new_owner`).
Internal function without access restriction.

[.contract-item]
[[OwnableComponent-_transfer_ownership]]
Emits an xref:OwnableComponent-OwnershipTransferred[OwnershipTransferred] event.

[.contract-item]
[[OwnableComponent-_propose_owner]]
Expand All @@ -243,10 +240,12 @@ Internal function without access restriction.
Emits an xref:OwnableComponent-OwnershipTransferStarted[OwnershipTransferStarted] event.

[.contract-item]
[[OwnableComponent-_transfer_ownership]]
==== `[.contract-item-name]#++_transfer_ownership++#++(ref self: ContractState, new_owner: ContractAddress)++` [.item-kind]#internal#
[[OwnableComponent-_accept_ownership]]
==== `[.contract-item-name]#++_accept_ownership++#++(ref self: ContractState)++` [.item-kind]#internal#

Transfers ownership to the pending owner. Resets pending owner to zero address.
Calls xref:OwnableComponent-_transfer_ownership[_transfer_ownership].

Transfers ownership of the contract to a new account (`new_owner`).
Internal function without access restriction.

Emits an xref:OwnableComponent-OwnershipTransferred[OwnershipTransferred] event.
Expand Down Expand Up @@ -323,7 +322,7 @@ Returns `true` if `account` has been granted `role`.
Returns the admin role that controls `role`. See {grant_role} and
{revoke_role}.

To change a role's admin, use {_set_role_admin}.
To change a role's admin, use {set_role_admin}.

[.contract-item]
[[IAccessControl-grant_role]]
Expand Down Expand Up @@ -439,7 +438,7 @@ accounts that have a role's admin role can call {grant_role} and {revoke_role}.
By default, the admin role for all roles is `DEFAULT_ADMIN_ROLE`, which means
that only accounts with this role will be able to grant or revoke other
roles. More complex role relationships can be created by using
{_set_role_admin}.
{set_role_admin}.

WARNING: The `DEFAULT_ADMIN_ROLE` is also its own admin: it has permission to
grant and revoke this role. Extra precautions should be taken to secure
Expand Down Expand Up @@ -488,7 +487,7 @@ accounts that have been granted it.

* xref:#AccessControlComponent-initializer[`++initializer(self)++`]
* xref:#AccessControlComponent-assert_only_role[`++assert_only_role(self, role)++`]
* xref:#AccessControlComponent-_set_role_admin[`++_set_role_admin(self, role, admin_role)++`]
* xref:#AccessControlComponent-set_role_admin[`++set_role_admin(self, role, admin_role)++`]
* xref:#AccessControlComponent-_grant_role[`++_grant_role(self, role, account)++`]
* xref:#AccessControlComponent-_revoke_role[`++_revoke_role(self, role, account)++`]
--
Expand Down Expand Up @@ -518,7 +517,7 @@ Returns `true` if `account` has been granted `role`.
Returns the admin role that controls `role`. See {grant_role} and
{revoke_role}.

To change a role's admin, use {_set_role_admin}.
To change a role's admin, use {set_role_admin}.

[.contract-item]
[[AccessControlComponent-grant_role]]
Expand Down Expand Up @@ -620,11 +619,13 @@ Initializes the contract by registering the xref:#IAccessControl[IAccessControl]
Panics if called by any account without the given `role`.

[.contract-item]
[[AccessControlComponent-_set_role_admin]]
==== `[.contract-item-name]#++_set_role_admin++#++(ref self: ContractState, role: felt252, admin_role: felt252)++` [.item-kind]#internal#
[[AccessControlComponent-set_role_admin]]
==== `[.contract-item-name]#++set_role_admin++#++(ref self: ContractState, role: felt252, admin_role: felt252)++` [.item-kind]#internal#

Sets `admin_role` as ``role``'s admin role.

Internal function without access restriction.

Emits a {RoleAdminChanged} event.

[.contract-item]
Expand Down
20 changes: 10 additions & 10 deletions docs/modules/ROOT/pages/api/erc1155.adoc
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

We're missing _update in the API

Copy link
Collaborator

Choose a reason for hiding this comment

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

And forgive me, I should have asked this before 😅 would it make sense to remove the underscore in _update for erc20 and erc1155? It seems weird that we're only doing it with erc721. The rationale, as I see it with erc1155, is to promote update_with_acceptance_check. I can get behind that if you want to keep it, but I'm not so sure with erc20. For consistency, we could just do update across the tokens. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think is ok to remove them.

Original file line number Diff line number Diff line change
Expand Up @@ -220,13 +220,13 @@ NOTE: See xref:#ERC1155Component-Hooks[Hooks] to understand how are hooks used.
--
.InternalImpl
* xref:#ERC1155Component-initializer[`++initializer(self, base_uri)++`]
* xref:#ERC1155Component-update[`++update(self, from, to, token_ids, values)++`]
* xref:#ERC1155Component-update_with_acceptance_check[`++update_with_acceptance_check(self, from, to, token_ids, values, data)++`]
* xref:#ERC1155Component-mint_with_acceptance_check[`++mint_with_acceptance_check(self, to, token_id, value, data)++`]
* xref:#ERC1155Component-batch_mint_with_acceptance_check[`++batch_mint_with_acceptance_check(self, to, token_ids, values, data)++`]
* xref:#ERC1155Component-burn[`++burn(self, from, token_id, value)++`]
* xref:#ERC1155Component-batch_burn[`++batch_burn(self, from, token_ids, values)++`]
* xref:#ERC1155Component-set_base_uri[`++set_base_uri(self, base_uri)++`]
* xref:#ERC1155Component-update_with_acceptance_check[`++update_with_acceptance_check(self, from, to, token_ids, values, data)++`]
* xref:#ERC1155Component-_set_base_uri[`++_set_base_uri(self, base_uri)++`]
* xref:#ERC1155Component-_update[`++_update(self, from, to, token_ids, values)++`]
--

[.contract-index]
Expand All @@ -253,13 +253,13 @@ for this purpose.
[[ERC1155Component-before_update]]
==== `[.contract-item-name]#++before_update++#++(ref self: ContractState, from: ContractAddress, to: ContractAddress, token_ids: Span<u256>, values: Span<u256>)++` [.item-kind]#hook#

Function executed at the beginning of the xref:#ERC1155Component-update[update] function prior to any other logic.
Function executed at the beginning of the xref:#ERC1155Component-_update[_update] function prior to any other logic.

[.contract-item]
[[ERC1155Component-after_update]]
==== `[.contract-item-name]#++after_update++#++(ref self: ContractState, from: ContractAddress, to: ContractAddress, token_ids: Span<u256>, values: Span<u256>)++` [.item-kind]#hook#

Function executed at the end of the xref:#ERC1155Component-update[update] function.
Function executed at the end of the xref:#ERC1155Component-_update[_update] function.

==== Embeddable functions

Expand Down Expand Up @@ -402,8 +402,8 @@ Initializes the contract by setting the token's base URI as `base_uri`, and regi
This should only be used inside the contract's constructor.

[.contract-item]
[[ERC1155Component-update]]
==== `[.contract-item-name]#++update++#++(ref self: ContractState, from: ContractAddress, to: ContractAddress, token_ids: Span<u256>, values: Span<u256>)++` [.item-kind]#internal#
[[ERC1155Component-_update]]
==== `[.contract-item-name]#++_update++#++(ref self: ContractState, from: ContractAddress, to: ContractAddress, token_ids: Span<u256>, values: Span<u256>)++` [.item-kind]#internal#

Transfers a `value` amount of tokens of type `id` from `from` to `to`.
Will mint (or burn) if `from` (or `to`) is the zero address.
Expand All @@ -425,7 +425,7 @@ See <<ERC1155Component-update_with_acceptance_check,update_with_acceptance_check
[[ERC1155Component-update_with_acceptance_check]]
==== `[.contract-item-name]#++update_with_acceptance_check++#++(ref self: ContractState, from: ContractAddress, to: ContractAddress, token_ids: Span<u256>, values: Span<u256>, data: Span<felt252>)++` [.item-kind]#internal#

Version of `update` that performs the token acceptance check by calling
Version of `_update` that performs the token acceptance check by calling
`onERC1155Received` or `onERC1155BatchReceived` in the receiver if
it implements `IERC1155Receiver`, otherwise by checking if it is an account.

Expand Down Expand Up @@ -494,8 +494,8 @@ Requirements:
Emits a <<ERC1155Component-TransferBatch,TransferBatch>> event.

[.contract-item]
[[ERC1155Component-set_base_uri]]
==== `[.contract-item-name]#++set_base_uri++#++(ref self: ContractState, base_uri: ByteArray)++` [.item-kind]#internal#
[[ERC1155Component-_set_base_uri]]
==== `[.contract-item-name]#++_set_base_uri++#++(ref self: ContractState, base_uri: ByteArray)++` [.item-kind]#internal#

Sets a new URI for all token types, by relying on the token type ID
substitution mechanism
Expand Down
12 changes: 6 additions & 6 deletions docs/modules/ROOT/pages/api/erc20.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,10 +220,10 @@ NOTE: See xref:#ERC20Component-Hooks[Hooks] to understand how are hooks used.
--
.InternalImpl
* xref:#ERC20Component-initializer[`++initializer(self, name, symbol)++`]
* xref:#ERC20Component-mint[`++mint(self, recipient, amount)++`]
* xref:#ERC20Component-burn[`++burn(self, account, amount)++`]
* xref:#ERC20Component-_transfer[`++_transfer(self, sender, recipient, amount)++`]
* xref:#ERC20Component-_approve[`++_approve(self, owner, spender, amount)++`]
* xref:#ERC20Component-_mint[`++_mint(self, recipient, amount)++`]
* xref:#ERC20Component-_burn[`++_burn(self, account, amount)++`]
* xref:#ERC20Component-_spend_allowance[`++_spend_allowance(self, owner, spender, amount)++`]
* xref:#ERC20Component-_update[`++_update(self, from, to, amount)++`]
--
Expand Down Expand Up @@ -396,8 +396,8 @@ Requirements:
- `spender` cannot be the zero address.

[.contract-item]
[[ERC20Component-_mint]]
==== `[.contract-item-name]#++_mint++#++(ref self: ContractState, recipient: ContractAddress, amount: u256)++` [.item-kind]#internal#
[[ERC20Component-mint]]
==== `[.contract-item-name]#++mint++#++(ref self: ContractState, recipient: ContractAddress, amount: u256)++` [.item-kind]#internal#

Creates an `amount` number of tokens and assigns them to `recipient`.

Expand All @@ -408,8 +408,8 @@ Requirements:
- `recipient` cannot be the zero address.

[.contract-item]
[[ERC20Component-_burn]]
==== `[.contract-item-name]#++_burn++#++(ref self: ContractState, account: ContractAddress, amount: u256)++` [.item-kind]#internal#
[[ERC20Component-burn]]
==== `[.contract-item-name]#++burn++#++(ref self: ContractState, account: ContractAddress, amount: u256)++` [.item-kind]#internal#

Destroys `amount` number of tokens from `account`.

Expand Down
Loading
Loading