feat: add erc1155 supply extension #418
Conversation
✅ Deploy Preview for contracts-stylus ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| /// Grants or revokes permission to `operator` | ||
| /// to transfer the caller's tokens, according to `approved`. | ||
| /// | ||
| /// Re-export of [`Erc1155::set_approval_for_all`] | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `&mut self` - Write access to the contract's state. | ||
| /// * `operator` - Account to add to the set of authorized operators. | ||
| /// * `approved` - Flag that determines whether or not permission will be | ||
| /// granted to `operator`. If true, this means `operator` will be allowed | ||
| /// to manage `msg::sender()`'s assets. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// * If `operator` is `Address::ZERO`, then the error | ||
| /// [`erc1155::Error::InvalidOperator`] is returned. | ||
| /// | ||
| /// # Requirements | ||
| /// | ||
| /// * The `operator` cannot be the `Address::ZERO`. | ||
| /// | ||
| /// # Events | ||
| /// | ||
| /// Emits an [`erc1155::ApprovalForAll`] event. |
There was a problem hiding this comment.
How about to implement IErc1155 trait here, to not copy paste doc comments from Erc1155. And for functions specific to supply extension like total_supply, exists and so on, just assume that user should expose them manually. Wdt?
There was a problem hiding this comment.
I would say that we should introduce IErc1155Supply trait that contains fns from both IErc1155 trait (re-exported due to limitation of single #[public] and required fns for Supply extension. Wdyt?
There was a problem hiding this comment.
@bidzyyys did I understand correctly, you mean something like:
pub trait IErc1155Supply {
/// comment...
fn total_supply
/// comment...
fn total_supply_all
/// comment...
fn exists
// IErc1155 fns
/// copy/pasted comment...
fn balance_of
/// copy/pasted comment...
fn balance_of_batch
// ...
}There was a problem hiding this comment.
But in this case we should copy paste doc comments all over again..
That is why I'm a bit against it. In future stylus sdk version we should be able to implement more then one #[public] trait. But for now I would vote for implementing the parental trait IErc1155 for this extension.
benches/src/erc1155_supply.rs
Outdated
There was a problem hiding this comment.
I am not sure if we need benches for each fn/contract, wdyt @qalisander?
I would say that for this contract we should check only totalSupply(uin256 id), totalSupply(), and exist(uint256 id), as the rest should be just re-export of base ERC-1155.
There was a problem hiding this comment.
All functions that use the underlying _update should be benched IMO, as the logic is different in erc1155-supply from the one in vanilla erc1155
There was a problem hiding this comment.
This is why I omitted benches for reexported getters and approval-related functions (they don't depend on the new _update)
There was a problem hiding this comment.
Idk. I see that having, that many benchmarks is not a big priority for now. Just to know how expensive or cheap a specific base contract is..
There was a problem hiding this comment.
@bidzyyys @qalisander wdyt is the best decision here then?
|
|
||
| /// Returns the value of tokens of type `id` owned by `account`. | ||
| /// | ||
| /// Re-export of [`Erc1155::balance_of`] |
There was a problem hiding this comment.
Tbh as we have comments like re-export of [Erc1155::balance_of] I would say that we do not need to write all the docs in terms of arguments, errors, events, etc. IMHO devs will be able to check based on the links. Wdyt @0xNeshi @qalisander @ggonzalez94 ?
There was a problem hiding this comment.
While I am in favor of avoiding copy/pasting comments for reexported functions, I do think it is more practical from dev point of view to be able to see what a function does immediately, e.g. by hovering (instead of having to click another link).
I would be in favor of having a macro (or some other tool) that takes a path to the reexported function, and copy/pastes the comment for us - we keep our code clean, but all the nice comment-related benefits are still there (docs on hover over function), wdyt? 🤔
| /// Grants or revokes permission to `operator` | ||
| /// to transfer the caller's tokens, according to `approved`. | ||
| /// | ||
| /// Re-export of [`Erc1155::set_approval_for_all`] | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `&mut self` - Write access to the contract's state. | ||
| /// * `operator` - Account to add to the set of authorized operators. | ||
| /// * `approved` - Flag that determines whether or not permission will be | ||
| /// granted to `operator`. If true, this means `operator` will be allowed | ||
| /// to manage `msg::sender()`'s assets. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// * If `operator` is `Address::ZERO`, then the error | ||
| /// [`erc1155::Error::InvalidOperator`] is returned. | ||
| /// | ||
| /// # Requirements | ||
| /// | ||
| /// * The `operator` cannot be the `Address::ZERO`. | ||
| /// | ||
| /// # Events | ||
| /// | ||
| /// Emits an [`erc1155::ApprovalForAll`] event. |
There was a problem hiding this comment.
I would say that we should introduce IErc1155Supply trait that contains fns from both IErc1155 trait (re-exported due to limitation of single #[public] and required fns for Supply extension. Wdyt?
|
@bidzyyys @qalisander does one of you know why the clippy CI step is registered as failing when all the individual steps are marked as "passed"? EDIT: moved the discussion to Slack |
Resolves #350
Built off of erc1155-supply. Due to major merge conflicts when trying to update the branch with
main, decided to create a new branch and manually copy/paste the specific changes.For the additional updates to the original, check out the commit messages.
PR Checklist