-
Notifications
You must be signed in to change notification settings - Fork 73
feat: add ERC-6909 Token and Supply extension #777
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for contracts-stylus ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hey, any update on this PR @0xNeshi ? |
Hey @onurinanc! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #777 +/- ##
==========================================
+ Coverage 95.46% 95.68% +0.22%
==========================================
Files 89 91 +2
Lines 15814 16793 +979
==========================================
+ Hits 15097 16069 +972
- Misses 717 724 +7 ☔ View full report in Codecov by Sentry. |
qalisander
left a comment
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.
@onurinanc great job and lgtm!
0xNeshi
left a comment
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.
🔥
| /// Re-export of [`Erc6909::_mint`]. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// * [`Error::InvalidReceiver`] - If the `to` address is [`Address::ZERO`]. | ||
| pub fn _mint( |
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.
This is not a re-export of Erc6909::_mint but a "re-implementation", maybe it makes sense to add missing doc sections:
https://github.com/OpenZeppelin/rust-contracts-stylus/pull/777/files#diff-c485e1cc294c36163cf1b14720fb9741e48b604c3db9c817def3aed1db955ed6R620
Same for other internal fns
| /// NOTE: This function is not virtual, {_update} should be overridden | ||
| /// instead. |
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.
Do these notes make sense in Stylus?
| /// receiver are [`Address::ZERO`], which means it cannot mint or burn | ||
| /// tokens. | ||
| /// | ||
| /// Relies on the `_update` mechanism. |
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.
nit: link to the _update fn
| /// Sets `amount` as the allowance of `spender` over the `owner`'s `id` | ||
| /// tokens. | ||
| /// | ||
| /// This internal function is equivalent to `approve`, and can be used to |
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.
nit: link to approve; check that other fn mentions are properly linked
| /// This internal function is equivalent to `approve`, and can be used to | ||
| /// e.g. set automatic allowances for certain subsystems, etc. |
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.
This sentence exists in Solidity too, but I'm not sure I really get its purpose.
|
|
||
| /// Approve `spender` to operate on all of `owner`'s tokens | ||
| /// | ||
| /// This internal function is equivalent to `setOperator`, and can be used |
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.
nit: fix name and link to actual fn
| /// Updates `owner`'s allowance for `spender` based on spent `amount`. | ||
| /// | ||
| /// Does not update the allowance value in case of infinite allowance. | ||
| /// Revert if not enough allowance is available. |
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.
| /// Revert if not enough allowance is available. |
already documented in # Errors
Co-authored-by: Nenad <[email protected]>
bidzyyys
left a comment
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.
First pass.
| - `SafeErc20` now implements: `try_safe_transfer`, `try_safe_transfer_from`, `transfer_and_call_relaxed`, `transfer_from_and_call_relaxed` and `approve_and_call_relaxed`. #765 | ||
| - Add bidirectional conversions between `ruint::Uint` and crypto library `Uint` types behind `ruint` feature toggle. #758 | ||
| - Add bidirectional conversions between `Uint` and `u8`, `u16`, `u32`, `u64`, `u128` types. #764 | ||
| - Add `Erc6909` contract and `Erc6909TokenSupply` extension. #777 |
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.
Should be added to "Unreleased" section.
| [[usage]] | ||
| == Usage | ||
|
|
||
| In order to make an xref:erc6909.adoc[ERC-6909] token with https://docs.rs/openzeppelin-stylus/0.3.0-alpha.1/openzeppelin_stylus/token/erc6909/extensions/supply/index.html[Supply] flavour, |
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.
We need to decide on version, but the one is wrong.
| While ERC-1155 introduced a multi-token interface capable of managing both fungible and non-fungible assets (such as ERC-20 and ERC-721) within a single smart contract, ERC-6909 streamlines this approach for greater efficiency by overcoming the limitations of ERC-1155 by removing contract-level callbacks and batch transfers, and by replacing the single-operator permission model with a hybrid allowance–operator system that enables more fine-grained token management. | ||
|
|
||
| The OpenZeppelin Stylus Contracts provides a complete implementation of the ERC-6909 standard. | ||
| On the https://docs.rs/openzeppelin-stylus/0.3.0-rc.1/openzeppelin_stylus/token/erc6909/struct.Erc6909.html[`API reference`] you'll find detailed information on their properties and usage. |
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.
Version issue here.
| #[entrypoint] | ||
| #[storage] | ||
| struct VaultManager { |
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 would use exactly the same code from the example; No VaultManager.
PR Checklist
ERC6909 Implementation in rust-contracts-stylus
Summary
This PR implements the ERC-6909 token and Supply extension in Rust for Arbitrum Stylus.
The following implementation is provided in this PR:
Comments
Integration tests and the benchmarks inside rust-contracts-stylus, which use nitro-testnode, didn't work with the M3 Pro Chip (would be an issue related to Docker). To solve the problem, firstly, [nitro-devnode] is tried to use. However, I observed that it is not suitable for the
e2e::testworkflow. So, an LTS Ubuntu machine is rented on AWS to run the integration tests and the benchmark.For the Supply Extension implementation,
Dereftraits are implemented. UsingDerefallows direct access to Erc6909 methods, improving readability and maintainability. However, deferencing operations might increase the gas. Designing Supply Token Extension in this way increases the abstraction and makes it easy to use for users who would like to use the Supply Extension with ERC-6909 Token Contract.Future Improvements
Currently, this PR does not include other ERC-6909 Extensions, specifically ContentURI and Metadata. Their example usage and the benchmarks should be added to the repository.
The unit tests that differentiate the
idshould be added.Benchmark
Benchmarks for ERC-6909 can be seen as follows: