Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Copy weight files into node-template #11098

Closed
wants to merge 5 commits into from

Conversation

tifecool
Copy link
Contributor

@tifecool tifecool commented Mar 23, 2022

PR attempts to solve issue #11090

Copied weight files and TOML from support.

polkadot address: 1s51dYXziQwTnSnModErKfra3TeEsVipqzY846CLCTEXtHJ

@tifecool tifecool changed the title Fix for issue #11090 Copy weight files into node-template Mar 23, 2022
@ggwpez ggwpez added the A3-in_progress Pull request is in progress. No review needed at this stage. label Mar 24, 2022
@ggwpez
Copy link
Member

ggwpez commented Mar 24, 2022

Thanks for the help!
You still need to use the weights, as currently the node is still using the weights from frame_support.
I did not mark this issue as "mentor" since there is more involved to what I read and it was more of a reminder for me.

Could you please rename the package to node-template-runtime-constants and move it to a constants dir? As more constants will be added there analogous to Polkadot. The structure should look exactly as in Polkadot.

@tifecool
Copy link
Contributor Author

I'm on it. Sorry about the late response.

@ggwpez
Copy link
Member

ggwpez commented Mar 25, 2022

That one point is still outstanding:

You still need to use the weights, as currently the node is still using the weights from frame_support.

I think we can also merge as-is and then I do that together with an update of the weights.
PS: The CI should be fixed when you merge master into your branch.

@tifecool
Copy link
Contributor Author

tifecool commented Mar 25, 2022

I think we can also merge as-is and then I do that together with an update of the weights.

I'd like this. Not sure I fully understand the code so well and would love to be involved 🙏

@shawntabrizi
Copy link
Member

/tip small

@substrate-tip-bot
Copy link

Please fix the following problems before calling the tip bot again:

  • Contributor did not properly post their Polkadot or Kusama address. Make sure the pull request has: "{network} address: {address}".

@shawntabrizi
Copy link
Member

@tifecool if you would like a small tip in KSM or DOT, please include your address in the PR description

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@tifecool
Copy link
Contributor Author

@tifecool if you would like a small tip in KSM or DOT, please include your address in the PR description

done ❤️

@shawntabrizi
Copy link
Member

/tip small

@substrate-tip-bot
Copy link

A small tip was successfully submitted for tifecool (1s51dYXziQwTnSnModErKfra3TeEsVipqzY846CLCTEXtHJ on polkadot).

https://polkadot.js.org/apps/#/treasury/tips

@ggwpez
Copy link
Member

ggwpez commented Mar 25, 2022

I thought about this again and think that it is better to not create a new crate, since we somehow need to keep the balance between completeness and understandability.
On the one hand it should be a complete example but on the other hand a new user should have a chance at understanding it.
So in the current state the weights are in their own folder besides the runtime lib.rs.

@ggwpez ggwpez added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Mar 25, 2022
@tifecool
Copy link
Contributor Author

I thought about this again and think that it is better to not create a new crate, since we somehow need to keep the balance between completeness and understandability. On the one hand it should be a complete example but on the other hand a new user should have a chance at understanding it. So in the current state the weights are in their own folder besides the runtime lib.rs.

So how should we implement it instead?

Comment on lines +47 to +48
// Re-export the non-default weights.
pub use weights::*;
Copy link
Member

Choose a reason for hiding this comment

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

Id rather we dont have this re-export for clarity where things are coming from

Copy link
Contributor Author

@tifecool tifecool Mar 25, 2022

Choose a reason for hiding this comment

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

Maybe we could rename the weights.rs to template_weights.rs instead? @ggwpez

@shawntabrizi
Copy link
Member

In general I don't think the node-template needs custom weight stuff. It adds quite a bit of complexity, and doesn't really make sense since users are likely to change the node template right away, and thus invalidate the weights.

Probably best to just get the default weights from the main substrate node at this time.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants