-
Notifications
You must be signed in to change notification settings - Fork 71
Add documentation for implementing EnumerableSet #786
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
…ustom storage types - Enhanced existing EnumerableSet docs with usage examples - Created comprehensive Antora documentation for custom storage types - Added note about StorageBytes/StorageString limitations - Updated navigation and cross-references Closes OpenZeppelin#761
✅ Deploy Preview for contracts-stylus ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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.
Very good work 🚀
Let's polish it a bit and we can merge
//! ``` | ||
//! extern crate alloc; | ||
//! | ||
//! use alloy_primitives::{Address, U256}; | ||
//! use stylus_sdk::prelude::*; | ||
//! use openzeppelin_stylus::utils::structs::enumerable_set::EnumerableSet; | ||
//! | ||
//! #[storage] | ||
//! struct MyContract { | ||
//! whitelist: EnumerableSet<Address>, | ||
//! } |
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.
//! ``` | |
//! extern crate alloc; | |
//! | |
//! use alloy_primitives::{Address, U256}; | |
//! use stylus_sdk::prelude::*; | |
//! use openzeppelin_stylus::utils::structs::enumerable_set::EnumerableSet; | |
//! | |
//! #[storage] | |
//! struct MyContract { | |
//! whitelist: EnumerableSet<Address>, | |
//! } | |
//! ```rust | |
//! extern crate alloc; | |
//! | |
//! use alloy_primitives::{Address, U256}; | |
//! use stylus_sdk::prelude::*; | |
//! use openzeppelin_stylus::utils::structs::enumerable_set::EnumerableSet; | |
//! | |
//! #[storage] | |
//! #[entrypoint] | |
//! struct MyContract { | |
//! whitelist: EnumerableSet<Address>, | |
//! } |
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.
Unresolving, still missing #[entrypoint]
.
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.
Unresolving, still missing #[entrypoint]
.
@Peponks9 I need to remind you to please not resolve change requests which have not been addressed. This is very important, as it makes for better clarity during code reviews. Only resolve change requests once:
- The change request is addressed (implemented).
- Your local changes are pushed to remote and PR is updated.
[[limitations]] | ||
== Current Limitations | ||
|
||
**Note:** `StorageBytes` and `StorageString` cannot currently be implemented for `EnumerableSet` due to limitations in the Stylus SDK. This limitations may change in future versions of the Stylus SDK. |
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.
Update the sentence to state that Bytes
and String
cannot currently be implemented due to SDK limitation. Link to the actual types.
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.
Made the changes but didn't find a link for String
, just Bytes
.
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.
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.
Both links added
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.
The changes haven't been made or haven't been pushed, you can resolve this conversation together with https://github.com/OpenZeppelin/rust-contracts-stylus/pull/786/files#r2281442336
[[testing]] | ||
== Testing Your Implementation |
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.
Remove this section, devs can't run OZ tests from within their projects
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.
Unresolving, the cargo test script is still present
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.
@Peponks9 another reminder not to resolve conversations that haven't been fully addressed and the changes pushed.
Add `rust` Co-authored-by: Nenad <[email protected]>
- Add comprehensive primitive type list with rustdoc links - Move detailed docs from module level to struct level - Update custom implementation guide with step-by-step breakdown - Fix documentation structure and formatting issues - Update limitations section with proper type links
- Add comprehensive primitive type list with rustdoc links - Move detailed docs from module level to struct level - Update custom implementation guide with step-by-step breakdown - Fix documentation structure and formatting issues - Update limitations section with proper type links
Hey @0xNeshi, changes made :) |
/// Sets have the following properties: | ||
/// | ||
/// * Elements are added, removed, and checked for existence in constant time | ||
/// (O(1)). | ||
/// * Elements are enumerated in O(n). No guarantees are made on the ordering. | ||
/// * Set can be cleared (all elements removed) in O(n). | ||
/// | ||
/// ## 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.
/// Sets have the following properties: | |
/// | |
/// * Elements are added, removed, and checked for existence in constant time | |
/// (O(1)). | |
/// * Elements are enumerated in O(n). No guarantees are made on the ordering. | |
/// * Set can be cleared (all elements removed) in O(n). | |
/// | |
/// ## Usage | |
/// Storage type for managing [sets] of primitive types. | |
/// | |
/// Sets have the following properties: | |
/// | |
/// * Elements are added, removed, and checked for existence in constant time | |
/// (O(1)). | |
/// * Elements are enumerated in O(n). No guarantees are made on the ordering. | |
/// * Set can be cleared (all elements removed) in O(n). | |
/// | |
/// [sets]: https://en.wikipedia.org/wiki/Set_(abstract_data_type) | |
/// | |
/// ## Usage |
@@ -188,6 +237,7 @@ mod tests { | |||
use stylus_sdk::prelude::TopLevelStorage; | |||
use alloy_primitives::private::proptest::{prop_assert, prop_assert_eq, proptest}; | |||
|
|||
|
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.
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 don't seem to find this one, on lines provided it is not after use alloy_primitives::private::proptest::{prop_assert, prop_assert_eq, proptest};
anymore.
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.
Maybe you didn't push local changes?
@@ -2,13 +2,6 @@ | |||
|
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.
Change the mod-level doc to:
//! Storage type for managing [sets] of primitive types.
//!
//! [sets]: https://en.wikipedia.org/wiki/Set_(abstract_data_type)
//! ``` | ||
//! extern crate alloc; | ||
//! | ||
//! use alloy_primitives::{Address, U256}; | ||
//! use stylus_sdk::prelude::*; | ||
//! use openzeppelin_stylus::utils::structs::enumerable_set::EnumerableSet; | ||
//! | ||
//! #[storage] | ||
//! struct MyContract { | ||
//! whitelist: EnumerableSet<Address>, | ||
//! } |
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.
Unresolving, still missing #[entrypoint]
.
//! You can implement `EnumerableSet` for your own storage types by implementing | ||
//! the `Element` and `Accessor` traits. See `element.rs` for trait definitions | ||
//! and the documentation for comprehensive examples. |
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.
Unresolving, as types for not converted into links.
[[limitations]] | ||
== Current Limitations | ||
|
||
**Note:** https://docs.rs/alloy-primitives/latest/alloy_primitives/struct.Bytes.html[`Bytes`] and `String` cannot currently be implemented for `EnumerableSet` due to limitations in the Stylus SDK. These limitations may change in future versions of the Stylus SDK. |
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.
**Note:** https://docs.rs/alloy-primitives/latest/alloy_primitives/struct.Bytes.html[`Bytes`] and `String` cannot currently be implemented for `EnumerableSet` due to limitations in the Stylus SDK. These limitations may change in future versions of the Stylus SDK. | |
https://docs.rs/alloy-primitives/latest/alloy_primitives/struct.Bytes.html[`Bytes`] and `String` cannot currently be implemented for `EnumerableSet` due to limitations in the Stylus SDK. These limitations may change in future versions of the Stylus SDK. |
nit: redundant, already in a separate section.
[[limitations]] | ||
== Current Limitations | ||
|
||
**Note:** https://docs.rs/alloy-primitives/latest/alloy_primitives/struct.Bytes.html[`Bytes`] and `String` cannot currently be implemented for `EnumerableSet` due to limitations in the Stylus SDK. These limitations may change in future versions of the Stylus SDK. |
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.
**Note:** https://docs.rs/alloy-primitives/latest/alloy_primitives/struct.Bytes.html[`Bytes`] and `String` cannot currently be implemented for `EnumerableSet` due to limitations in the Stylus SDK. These limitations may change in future versions of the Stylus SDK. | |
**Note:** https://docs.rs/alloy-primitives/latest/alloy_primitives/struct.Bytes.html[`Bytes`] and https://doc.rust-lang.org/stable/alloc/string/struct.String.html[`String`] cannot currently be implemented for `EnumerableSet` due to limitations in the Stylus SDK. These limitations may change in future versions of the Stylus SDK. |
[[testing]] | ||
== Testing Your Implementation |
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.
Unresolving, the cargo test script is still present
#[storage] | ||
struct AccessControl { | ||
role_members: StorageMap<B256, EnumerableSet<Address>>, | ||
} | ||
|
||
impl AccessControl { | ||
fn grant_role(&mut self, role: B256, account: Address) { | ||
self.role_members.get(role).add(account); | ||
} | ||
|
||
fn revoke_role(&mut self, role: B256, account: Address) { | ||
self.role_members.get(role).remove(account); | ||
} | ||
|
||
fn get_role_members(&self, role: B256) -> Vec<Address> { | ||
self.role_members.get(role).values() | ||
} | ||
} |
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.
Provide a full working contract example, including imports. Devs should be able to copy/paste this example and have it deployable out of the box.
[source,rust] | ||
---- | ||
#[storage] | ||
struct Whitelist { |
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 for this, make it fully working example
Also, fix the failing doc CI. The other CI jobs seem to be failing due to some issue with |
@0xNeshi yes, still haven’t pushed changes and some errors still to be addressed. But didn’t some progress today ñ. |
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.
All the active conversations need to be addressed. For future reference, feel free to just make the necessary change requests or discuss them in the conversation threads, but leave it to the maintainers to actually resolve the conversations.
Hey @Peponks9 , just checking in to see if you need any help addressing the issues |
Description
This PR addresses issue #761 by adding the documentation for implementing
EnumerableSet
with user-defined storage types in Stylus.Changes Made
Added Examples to Existing EnumerableSet Documentation
EnumerableSet<Address>
incontracts/src/utils/structs/enumerable_set/mod.rs
Element
andAccessor
traitsCreated Antora Documentation
docs/modules/ROOT/pages/enumerable-set-custom.adoc
User
structUpdated Navigation and References
docs/modules/ROOT/nav.adoc
docs/modules/ROOT/pages/utilities.adoc
with cross-referenceAddressed Limitations
StorageBytes
andStorageString
cannot currently be implemented due to Stylus SDK limitationsTesting
npm run docs
Cargo test
successfully runRelated Issue
Closes #761
@0xNeshi