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

Girazoki xcm max ump weight and transact info migration #1114

Merged

Conversation

girazoki
Copy link
Collaborator

@girazoki girazoki commented Dec 22, 2021

What does it do?

Attempt to perform an migration on the TransactInfo storage item. I had to modify the migrations pallet to accept a trait that is implemented for tuples, as this migration should only be applied to moonbase and moonriver runtimes

In particular, the TransactInfo is changed in the following ways:

  • A max transact weight is added, so that we can prevent transactors from stalling the queue in the dest chain. This was previously hardcoded in the runtime, so now we make it accessible and changeable in a storage item
  • fee_per_byte, base_weight and metadata_size are removed because those parameters only make sense when executing local transacitons in the relay, not transactions through xcm. For xcm, only the fee_per_weight is accounted. This has been confirmed in our Alphanet by Lido, where we have set to 0 all the non-relevant parameters and everything has been working so far.
  • fee_per_weight has been changed to fee_per_second, to better reflect cases (like Kusama) in which the fee per weight unit is lower than one.

What important points reviewers should know?

Is there something left for follow-up PRs?

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?

@girazoki girazoki changed the base branch from master to girazoki-fix-try-runtime-pallet-maintenance December 22, 2021 11:15
@girazoki girazoki added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes A0-pleasereview Pull request needs code review. labels Dec 22, 2021
Base automatically changed from girazoki-fix-try-runtime-pallet-maintenance to master December 23, 2021 16:00
@girazoki girazoki added the D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited label Jan 12, 2022
@girazoki girazoki requested a review from notlesh January 14, 2022 10:41
@librelois
Copy link
Collaborator

@girazoki did you test the migration with try-runtime?

@girazoki
Copy link
Collaborator Author

girazoki commented Jan 14, 2022

Yes, but I will try one more time before merging because I changed the fee_per_Second

@librelois librelois mentioned this pull request Jan 14, 2022
32 tasks
Copy link
Contributor

@notlesh notlesh left a comment

Choose a reason for hiding this comment

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

LGTM, although I think this may not be the end of integer resolution problems in this code. I'm opening an internal ticket to review this later.

@girazoki girazoki merged commit ffb44f0 into master Jan 14, 2022
@girazoki girazoki deleted the girazoki-xcm-max-ump-weight-and-transact-info-migration branch January 14, 2022 17:40
@notlesh notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Mar 16, 2022
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 D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants