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

Fix non-deterministic build (use paste! instead of gensym!) #1328

Merged
merged 2 commits into from
Mar 1, 2022

Conversation

nanocryk
Copy link
Contributor

@nanocryk nanocryk commented Mar 1, 2022

What does it do?

In the ERC20 balances precompile we impl storage prefixes for each possible instance of pallet_balances. To avoid name clashes and long macro lines we make a dedicated module for each instance in which we create the storage prefix struct.

Before I used gensym! to generate a unique name for the module, but it's non-deterministic and thus cause issues with sr-tool.
This PR uses paste! instead to generate a module name from the instance struct name, providing a unique but deterministic name.

What important points reviewers should know?

Is there something left for follow-up PRs?

Maybe concatenate the literal automatically, and even loop throught 0 to 16 to avoid calling the macro multiple times?

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

@nanocryk nanocryk added A0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D3-trivial PR contains trivial changes in a runtime directory that do not require an audit labels Mar 1, 2022
@nanocryk nanocryk requested a review from librelois March 1, 2022 13:42
precompiles/balances-erc20/src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Éloïs <c@elo.tf>
@crystalin
Copy link
Collaborator

Isn't a migration required for this ?

@librelois
Copy link
Collaborator

Isn't a migration required for this ?

No, it's compile-time stuff only

@nanocryk
Copy link
Contributor Author

nanocryk commented Mar 1, 2022

Isn't a migration required for this ?

It doesn't change any storage key. The only change is the name of the Rust module containing the struct, which have no impact on the storage. However for building the previous code generated different module names each time you compiled, which broke sr-tool.

@nanocryk nanocryk merged commit 13b59ff into master Mar 1, 2022
@nanocryk nanocryk deleted the jeremy-fix-non-deterministic-build branch March 1, 2022 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D3-trivial PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants