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.

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

ERC20Votes docs #962

Merged
merged 52 commits into from
Apr 16, 2024
Merged

Conversation

ericnordelo
Copy link
Member

Partially fixes #815

PR Checklist

  • Tests
  • Documentation
  • Added entry to CHANGELOG.md
  • Tried the feature on a public network

ericnordelo and others added 30 commits March 5, 2024 14:53
Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

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

The docs are looking good, Eric! I left some comments and questions

docs/modules/ROOT/nav.adoc Outdated Show resolved Hide resolved
src/utils.cairo Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

I get not documenting structs because of the forthcoming native StorageArray; however, I feel like we should at least mention that this code is temporary. I'm not sure where exactly. Thoughts? Let's also not forget to track this as an issue once we're merged

And math should be stable enough to document, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

math documented!

Regarding Checkpoint and Trace I don't think we need to document them, since they are not intended as part of the library public API, they are just helpers for the erc20votes extension. Maybe we can move them outside of the utilities folder.

Regarding StorageArray, I don't think is worth documenting it since we don't want to support it, but to remove it in favor of the native implementation. Not sure where to mention that StorageArray is a temporal solution either, I vote we leave it for a different issue as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

docs/modules/ROOT/pages/api/erc20.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/erc20.adoc Outdated Show resolved Hide resolved

[.contract-item]
[[ERC20VotesComponent-DelegateChanged]]
==== `[.contract-item-name]#++DelegateChanged++#++([key] delegator: ContractAddress, [key] from_delegate: ContractAddress, [key] to_delegate: ContractAddress)++` [.item-kind]#event#
Copy link
Collaborator

Choose a reason for hiding this comment

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

The [key] is not visually appealing :( though I think this should be included. For the sake of consistency, how do you feel about making this a separate issue so we can tackle all of the APIs at once?

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 we should prioritize showing which keys are indexed, but agree maybe is better for a different issue, since we are releasing next week.

Copy link
Member Author

Choose a reason for hiding this comment

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

src/utils/structs/checkpoint.cairo Outdated Show resolved Hide resolved
src/utils/structs/checkpoint.cairo Outdated Show resolved Hide resolved
src/utils/structs/checkpoint.cairo Outdated Show resolved Hide resolved
src/utils/structs/checkpoint.cairo Outdated Show resolved Hide resolved
src/utils/structs/checkpoint.cairo Outdated Show resolved Hide resolved
Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

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

Also, the readme code block needs ERC20HooksEmptyImpl

Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

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

Left a final two suggestions (it's really just one). Otherwise, I think this is good to go!

docs/modules/ROOT/pages/api/erc20.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/erc20.adoc Outdated Show resolved Hide resolved
@ericnordelo ericnordelo merged commit cbd76fc into OpenZeppelin:main Apr 16, 2024
2 checks passed
@ericnordelo ericnordelo deleted the feat/erc20-votes-docs branch April 16, 2024 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate ERC20Votes to component
2 participants