feture: refactored ERC6909#284
Conversation
👷 Deploy request for compose-diamonds pending review.Visit the deploys page to approve it
|
maxnorm
left a comment
maxnorm
left a comment
There was a problem hiding this comment.
You can remove the redundant subfolder ERC6909. The new approach make it easier to add new features to a standard by only defining a new folder with the feature name under one main ERC6909 folder
| * @dev Emits a {Transfer} event to the zero address. | ||
| * @param _amount The amount of tokens to burn. | ||
| */ | ||
| function burnERC6909(uint256 _id, uint256 _amount) external { |
There was a problem hiding this comment.
No need for ERC6909 in the selector
| * @param _account The address whose tokens will be burned. | ||
| * @param _amount The amount of tokens to burn. | ||
| */ | ||
| function burnERC6909From(address _account, uint256 _id, uint256 _amount) external { |
There was a problem hiding this comment.
- Same here for the selector (no
ERC6909) - Use
_frominstead of_account - add a
InvalidSendererror for_from == address(0)
| function burnERC6909From(address _account, uint256 _id, uint256 _amount) external { | ||
| ERC6909Storage storage s = getStorage(); | ||
| uint256 currentAllowance = s.allowance[_account][msg.sender][_id]; | ||
| uint256 fromBalance = s.balanceOf[msg.sender][_id]; |
There was a problem hiding this comment.
Make sure check the right balance (currently, the fromBalance use the caller balance)
| } | ||
| unchecked { | ||
| s.allowance[_account][msg.sender][_id] = currentAllowance - _amount; | ||
| s.balanceOf[_account][_id] = fromBalance - _amount; |
There was a problem hiding this comment.
- Move the balance update after the allowance check.
- Check if the balance is sufficient for the amount burned
You can refer to the current burn & transfer in the ERC6909Mod
| s.balanceOf[_account][_id] = fromBalance - _amount; | ||
| } | ||
| } | ||
| emit Transfer(_account, msg.sender, address(0), _id, _amount); |
There was a problem hiding this comment.
reverse order
event Transfer(
address _caller, address indexed _sender, address indexed _receiver, uint256 indexed _id, uint256 _amount
);Here the _caller = msg.sender and the _sender = _from/_account
| * @param _account The address whose tokens will be burned. | ||
| * @param _amount The amount of tokens to burn. | ||
| */ | ||
| function burnERC6909From(address _account, uint256 _id, uint256 _amount) { |
| * @param _value The number of tokens to mint. | ||
| */ | ||
| function mintERC6909(address _account, uint256 _id, uint256 _value) { | ||
| ERC6909Storage storage s = getStorage(); |
There was a problem hiding this comment.
get storage after the receiver check. this avoid unneccery storage access if we revert
| * @param _account The address receiving the newly minted tokens. | ||
| * @param _value The number of tokens to mint. | ||
| */ | ||
| function mintERC6909(address _account, uint256 _id, uint256 _value) { |
There was a problem hiding this comment.
remove ERC6909 in selector
|
Will make the changes and revert before the day ends |
|
Hi sir, @maxnorm could you review again? |
2 participants
Summary
Changes Made
Checklist
Before submitting this PR, please ensure:
Code follows the Solidity feature ban - No inheritance, constructors, modifiers, public/private variables, external library functions,
using fordirectives, orselfdestructCode follows Design Principles - Readable, uses diamond storage, favors composition over inheritance
Code matches the codebase style - Consistent formatting, documentation, and patterns (e.g. ERC20Facet.sol)
Code is formatted with
forge fmtExisting tests pass - Run tests to be sure existing tests pass.
New tests are optional - If you don't provide tests for new functionality or changes then please create a new issue so this can be assigned to someone.
All tests pass - Run
forge testand ensure everything worksDocumentation updated - If applicable, update relevant documentation
Make sure to follow the contributing guidelines.
Additional Notes