-
Notifications
You must be signed in to change notification settings - Fork 4
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
Conversation
…ermine parameters that differentiate templates
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. |
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. |
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.
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.
@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 |
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. |
Both templates are compiling with the latest macros commit, see OpenZeppelin/polkadot-runtime-templates#300 (comment) |
Still updating in-line docs and the README in this PR to follow review suggestions |
Need to re-review xtokens, xcm-weight-trader, and xcm-transactor configurations in templates tomorrow with fresh eyes before final merge |
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.
Looks good! Left some suggestions for the module comments, feel free to take them if you didn't work on them already
Co-authored-by: Gustavo Gonzalez <ggonzalezsomer@gmail.com>
Co-authored-by: Gustavo Gonzalez <ggonzalezsomer@gmail.com>
…ither template not included in the macros
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 |
Implements wrapper macros for all pallet groupings
PR using macros in both runtime templates
Final TODOs:
Tanssi fixes:
Patches merged into macros that should be reviewed closely:
v2
back intomain
polkadot-runtime-templates#333Follow ups