-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Update transfer token gas and fee #7947
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
Merged
acatangiu
merged 31 commits into
paritytech:master
from
alistair-singh:alistair/transfer-token-gas
Apr 16, 2025
Merged
Changes from all commits
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
be27497
Update transfer token gas and fee
alistair-singh d92de78
add prdoc
alistair-singh 87b03e1
Merge branch 'master' into alistair/transfer-token-gas
alistair-singh 7a4bc44
revert mint
alistair-singh 4a49ce0
update prdoc
alistair-singh c5d92d0
Update prdoc/pr_7947.prdoc
alistair-singh 7f18465
Merge branch 'master' into alistair/transfer-token-gas
alistair-singh 364145f
upgrade gas for v2
alistair-singh 72a99c3
empty migration
alistair-singh 2882fac
fix migration
alistair-singh d8a02dc
add storage version
alistair-singh 4b1e42d
update pr doc
alistair-singh c7b6e53
Implement the halfing of fee per gas
alistair-singh 8abfa3e
Fix weights
alistair-singh c2488ae
add post and pre migration checks
alistair-singh fd6a265
more accurate name
alistair-singh f994615
Merge branch 'master' into alistair/transfer-token-gas
alistair-singh d8b25e8
fix clippy
alistair-singh 720a12e
fix semver
alistair-singh c760a58
copy paste error
alistair-singh cb25929
check v1 and v2
alistair-singh 2725b03
updated comments
alistair-singh 76ed37f
Merge branch 'master' into alistair/transfer-token-gas
alistair-singh 083bd3b
Fix breaking test
yrong a51b21d
Merge pull request #1 from yrong/ron/transfer-token-gas
alistair-singh 2f528f2
Merge branch 'master' into alistair/transfer-token-gas
alistair-singh f598dc8
fix pr doc
alistair-singh 4248c8e
fix bridge hub westend tests
alistair-singh dd76151
Merge branch 'master' into alistair/transfer-token-gas
alistair-singh cf05cf4
Merge branch 'master' into alistair/transfer-token-gas
alistair-singh b186a68
Merge branch 'master' into alistair/transfer-token-gas
alistair-singh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -661,7 +661,7 @@ pub mod bridging { | |
| /// (initially was calculated by test `OutboundQueue::calculate_fees` - ETH/ROC 1/400 and fee_per_gas 20 GWEI = 2200698000000 + *25%) | ||
| /// Needs to be more than fee calculated from DefaultFeeConfig FeeConfigRecord in snowbridge:parachain/pallets/outbound-queue/src/lib.rs | ||
| /// Polkadot uses 10 decimals, Kusama and Rococo 12 decimals. | ||
| pub const DefaultBridgeHubEthereumBaseFee: Balance = 2_750_872_500_000; | ||
| pub const DefaultBridgeHubEthereumBaseFee: Balance = 3_833_568_200_000; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could probably add the value here and then reuse: https://github.com/paritytech/polkadot-sdk/blob/master/cumulus/parachains/runtimes/constants/src/westend.rs#L172 Not a major issue though |
||
| pub storage BridgeHubEthereumBaseFee: Balance = DefaultBridgeHubEthereumBaseFee::get(); | ||
| pub SiblingBridgeHubWithEthereumInboundQueueInstance: Location = Location::new( | ||
| 1, | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| title: Snowbridge - Update TransferToken command gas limit. | ||
|
|
||
| doc: | ||
| - audience: Runtime Dev | ||
| description: | | ||
| Transfering certain ERC20 tokens require more gas than 100_000 gas. An example is LDO token which requires 140_000 gas. | ||
| This change updates the gas limit to 200_000 and also updates the default fees for testnet runtimes. | ||
alistair-singh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| NOTE: make sure to update the relevant runtime fees to account for this change. | ||
|
|
||
| crates: | ||
| - name: asset-hub-westend-runtime | ||
| bump: patch | ||
| - name: asset-hub-rococo-runtime | ||
| bump: patch | ||
| - name: bridge-hub-westend-runtime | ||
| bump: patch | ||
| - name: bridge-hub-rococo-runtime | ||
| bump: patch | ||
| - name: snowbridge-outbound-queue-primitives | ||
| bump: patch | ||
| - name: snowbridge-pallet-system | ||
| bump: minor | ||
| - name: snowbridge-pallet-outbound-queue-v2 | ||
| bump: minor | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Are these constants network-dependent? Could they possibly differ between the ETH testnet and production? Is it likely that we’ll change them again in the future?
I think it's not a common pattern to use versioned storage migration just for changing values rather than the structure of the storage.
I'm wondering if it would make sense for the
ConstantGasMeterto be provided by the runtimes - in other words, extracted from theoutbound-queuecrate and placed directly into the runtimes.That way, next time we wouldn’t need to touch the
polkadot-sdkcrates - we could just update the fellows runtimes. We can add migrations directly in the runtime. Overall, for changing fee/gas constants, we should only need to do a patch release of the fellows runtimes, without touching thepolkadot-sdkcrates.Actuall, I think in this case, we could create
ConstantGasMetercopy in fellows and add migrations directly there, so we are not blocked in this case.I don’t want to change it in this PR, but previously we had a similar issue with the Electra stuff, where some configuration was hard-coded in
polkadot-sdkwhen it should have been externalized.My intention here is to externalize those hard-coded configurations and constants so they can be provided by the runtime. This way, we can update and release production runtimes more easily, without having to wait for a stable
polkadot-sdkrelease - we can just make the changes directly when needed.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.
No, this is the upper bound on gas usage for the action on the Ethereum side. And this is stable per runtime because the EVM is the same across bridge instances (paseo-sepolia, westend-sepolia, polkadot-mainnet) and the same contract will consume the same amount of gas regardless of the Ethereum chain. The gas price or fee_per_gas can change per bridge instance but that is stored in pallet storage and can be updated with
set_pricing_parametersextrinsic.