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

Pallet grouping macros #1

Merged
merged 73 commits into from
Nov 4, 2024
Merged

Pallet grouping macros #1

merged 73 commits into from
Nov 4, 2024

Conversation

4meta5
Copy link
Collaborator

@4meta5 4meta5 commented Aug 26, 2024

@4meta5 4meta5 self-assigned this Aug 27, 2024
src/system.rs Outdated Show resolved Hide resolved
@4meta5 4meta5 changed the title System Wrapper Macro Pallet grouping macros Sep 30, 2024
@OpenZeppelin OpenZeppelin deleted a comment from ozgunozerk Oct 31, 2024
@4meta5
Copy link
Collaborator Author

4meta5 commented Oct 31, 2024

Scope for this PR is closed and #8 is clearly a follow up. Going forward I would appreciate a constructive review that actually checks that all the patches were properly included.

@4meta5
Copy link
Collaborator Author

4meta5 commented Oct 31, 2024

The remaining work that needs to be focused on in this PR is adding xtokens and xcm-transactor. We need to focus on these changes before anything else.

Copy link
Collaborator

@ggonzalez94 ggonzalez94 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 few nits and suggestions, especially around comments and how we document the macro. I think:

  • We should step up our game in terms of in-line docs, each grouping should have an explanation of what it does and what pallets it uses(I tried to provide an example for assets)
  • I saw a lot of inconsistencies in comments(/// and //), these were probably porter over from the templates repo, but I think it is a good opportunity to fix them.

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/assets.rs Outdated Show resolved Hide resolved
src/assets.rs Show resolved Hide resolved
@4meta5
Copy link
Collaborator Author

4meta5 commented Oct 31, 2024

@ggonzalez94 I did not add any comments to the code, these all existed in our templates. Let's address code comments inside the expansion in a follow up so that I can focus on merging in all of the work that has been only merged into the templates today. #24

@4meta5
Copy link
Collaborator Author

4meta5 commented Nov 1, 2024

Currently updating templates PR to use the most recent commit (OpenZeppelin/polkadot-runtime-templates#300 (comment)). Need to validate the last commit works for both templates before merging.

@4meta5
Copy link
Collaborator Author

4meta5 commented Nov 3, 2024

Both templates are compiling with the latest macros commit, see OpenZeppelin/polkadot-runtime-templates#300 (comment)

@4meta5
Copy link
Collaborator Author

4meta5 commented Nov 3, 2024

Still updating in-line docs and the README in this PR to follow review suggestions

@4meta5
Copy link
Collaborator Author

4meta5 commented Nov 3, 2024

Need to re-review xtokens, xcm-weight-trader, and xcm-transactor configurations in templates tomorrow with fresh eyes before final merge

Copy link
Collaborator

@ggonzalez94 ggonzalez94 left a comment

Choose a reason for hiding this comment

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

Looks good! Left some suggestions for the module comments, feel free to take them if you didn't work on them already

src/consensus.rs Outdated Show resolved Hide resolved
src/evm.rs Show resolved Hide resolved
src/consensus.rs Outdated Show resolved Hide resolved
src/evm.rs Show resolved Hide resolved
src/governance.rs Outdated Show resolved Hide resolved
src/system.rs Outdated Show resolved Hide resolved
src/xcm.rs Outdated Show resolved Hide resolved
4meta5 and others added 7 commits November 3, 2024 18:29
Co-authored-by: Gustavo Gonzalez <ggonzalezsomer@gmail.com>
Co-authored-by: Gustavo Gonzalez <ggonzalezsomer@gmail.com>
@4meta5
Copy link
Collaborator Author

4meta5 commented Nov 4, 2024

This PR is ready to be merged, but I am not completely satisfied with the README. One point of confusion right now is that the repository is polkadot-runtime-wrappers and the crate name is openzeppelin-polkadot-wrappers. This comes up a few times and I think it would be more clear if the repository was renamed to match the crate name. @ggonzalez94 let me know what you think and maybe we can create a follow up issue to rename the repo and update the README to clear any confusion around this

@4meta5 4meta5 merged commit 89023af into main Nov 4, 2024
4 checks passed
@4meta5 4meta5 deleted the amar-oz-system-wrapper-poc branch November 4, 2024 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
4 participants