-
Notifications
You must be signed in to change notification settings - Fork 141
Upgrade to unstable2507 #1029
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
base: main
Are you sure you want to change the base?
Upgrade to unstable2507 #1029
Conversation
|
|
||
| impl pallet_transaction_payment::Config for Runtime { | ||
| type RuntimeEvent = RuntimeEvent; | ||
| type OnChargeTransaction = impls::tx_payment::FungiblesAdapter< |
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.
Reasoning for this change: Using FungiblesAdapter (plural) for NativeAndAsset and DotLocation is the same as using FungibleAdapter (singular) with Balances because NativeAndAsset will just forward all calls to Balances for DotLocation.
| pallet-election-provider-multi-block = { version = "0.3.4", default-features = false } | ||
| pallet-staking-async = { version = "0.6.2", default-features = false } | ||
| pallet-election-provider-multi-block = { version = "0.5.0", default-features = false } | ||
| pallet-staking-async = { version = "0.7.0", default-features = false } |
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.
@kianenigma anything to do when bumping these?
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 changes needed for this bump.
| type OnChargeTransaction = | ||
| pallet_transaction_payment::FungibleAdapter<Balances, ResolveTo<StakingPot, Balances>>; | ||
| type WeightToFee = WeightToFee; | ||
| type WeightToFee = pallet_revive::evm::fees::BlockRatioFee<333333333, 10_815_700_000_000, Self>; |
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.
Where do these numbers come from?
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.
This encodes the same fraction as the one in the WeightToFeePolynomial used in WeightToFee, which is defined as:
- numerator:
super::currency::CENTS- this is
1_000_000_000_000 / 30 / 100=333_333_333
- this is
- denominator:
100 * Balance::from(ExtrinsicBaseWeight::get().ref_time())- this is
100 * 1_000 * 108_157=10_815_700_000
- this is
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.
I corrected the value that was here before, it was wrongly calculated.
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.
Maybe you can add a sanity check test to ensure that the fee of a simple transaction is sane?
And please put the calculation into a comment above the code so that we can reproduce it 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.
I added a comment with explanation.
|
/cmd bench --runtime asset-hub-kusama --pallet pallet_revive |
system-parachains/asset-hubs/asset-hub-kusama/src/weights/pallet_revive.rs
Outdated
Show resolved
Hide resolved
|
Command "bench --runtime asset-hub-kusama --pallet pallet_revive" has started 🚀 See logs here |
|
Command "bench --runtime asset-hub-kusama --pallet pallet_revive" has finished ✅ See logs here Subweight results:No changes found. |
|
/cmd fmt |
| type OnChargeTransaction = | ||
| pallet_transaction_payment::FungibleAdapter<Balances, ResolveTo<StakingPot, Balances>>; | ||
| type WeightToFee = WeightToFee; | ||
| type WeightToFee = pallet_revive::evm::fees::BlockRatioFee<333333333, 10_815_700_000, Self>; |
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.
can you add a comment here explaining the used WeightToFee? I'm sure it will not age well without clear docs/code-comments..
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.
I added a comment with explanation.
| type DepositPerChildTrieItem = DepositPerChildTrieItem; | ||
| type DepositPerByte = DepositPerByte; | ||
| type WeightPrice = pallet_transaction_payment::Pallet<Self>; | ||
| // TODO(#840): use `weights::pallet_revive::WeightInfo` here |
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.
with the new pallet-revive version you should be able to fix this too, right?
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.
I believe the benchmark is still not fixed. Also we need solc to be installed on the benchmarking bot. We don't have time to get this done right now.
| snowbridge-outbound-queue-runtime-api = { version = "0.17.0", default-features = false } | ||
| snowbridge-outbound-queue-v2-runtime-api = { version = "0.6.0", default-features = false } | ||
| snowbridge-outbound-queue-primitives = { version = "0.6.0", default-features = false } | ||
| snowbridge-pallet-ethereum-client = { version = "0.17.0", default-features = false } | ||
| snowbridge-pallet-inbound-queue = { version = "0.17.0", default-features = false } | ||
| snowbridge-pallet-inbound-queue-v2 = { version = "0.6.0", default-features = false } | ||
| snowbridge-pallet-inbound-queue-fixtures = { version = "0.25.0" } | ||
| snowbridge-pallet-ethereum-client-fixtures = { version = "0.25.0" } | ||
| snowbridge-pallet-outbound-queue = { version = "0.17.0", default-features = false } | ||
| snowbridge-pallet-outbound-queue-v2 = { version = "0.6.0", default-features = false } | ||
| snowbridge-pallet-system = { version = "0.17.0", default-features = false } | ||
| snowbridge-pallet-system-v2 = { version = "0.6.0", default-features = false } | ||
| snowbridge-pallet-system-frontend = { version = "0.6.0", default-features = false } | ||
| snowbridge-inbound-queue-primitives = { version = "0.6.0", default-features = false } | ||
| snowbridge-runtime-common = { version = "0.18.0", default-features = false } | ||
| snowbridge-runtime-test-common = { version = "0.20.0" } | ||
| snowbridge-system-runtime-api = { version = "0.17.0", default-features = false } | ||
| snowbridge-system-v2-runtime-api = { version = "0.6.0", default-features = false } |
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.
@claravanstaden please confirm the snowbridge pallet versions bump
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.
All of the version bumps are just because of crate dependency changes.
|
Command "fmt" has started 🚀 See logs here |
|
Command "fmt" has failed ❌! See logs here |
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.
To bump to the latest: https://matrix.to/#/!cqAmzdIcbOFwrdrubV:parity.io/$Kygjlcxk58wiMVkbVeiK1mAfZtLnkB8i-oVBPnoIb5I?via=parity.io&via=matrix.org&via=web3.foundation
The other Snowbridge crate bumps are correct, its just because the Snowbridge crate dependency updates.
I bumped that crate. |
|
Review required! Latest push from author must always be reviewed |
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.
Found 5 issues: CI coverage risk from skipping revive fixtures, potential runtime behavior change in Asset Hub Polkadot fee charging, and several hard-coded economic/weight constants in Asset Hub Kusama that may silently drift when upstream constants change.
Suggestions that couldn't be attached to a specific line
.github/workflows/test.yml:108, 134
SKIP_PALLET_REVIVE_FIXTURES: 1 is applied to both cargo test --all-features and benchmarking runs. This reduces signal from CI exactly in the area being upgraded (revive/EVM). If skipping fixtures is required for performance/stability, consider limiting it to benchmarking only, or adding a separate test job that runs without SKIP_PALLET_REVIVE_FIXTURES (even if allowed to be slower) to keep upgrade safety.
system-parachains/asset-hubs/asset-hub-kusama/src/lib.rs:1690, 1713
pallet_revive::evm::tx_extension::SetOrigin was added to both the normal TxExtension tuple and the Ethereum EthExtra tuple. If SetOrigin is meant only for Ethereum transactions, including it in the global TxExtension risks affecting (or at least adding overhead to) all extrinsics, and could introduce subtle origin/validation differences for non-EVM calls. Confirm SetOrigin is a no-op for non-Ethereum extrinsics; otherwise, move it exclusively into the Ethereum-specific EthExtra path (or gate it behind an extension wrapper that only runs for Ethereum extrinsic types).
| env: | ||
| RUSTFLAGS: "-D warnings" | ||
| SKIP_WASM_BUILD: 1 | ||
| SKIP_PALLET_REVIVE_FIXTURES: 1 |
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.
Setting SKIP_PALLET_REVIVE_FIXTURES: 1 disables fixture-related work during clippy. If fixture generation/validation is part of catching breaking changes in pallet-revive (especially around EVM backend), this can mask regressions. Consider scoping this env var only to jobs where fixtures are irrelevant, or add a dedicated (possibly nightly/optional) job that still runs fixture generation/validation so upgrades don’t silently break it.
| // The two generic parameters of `BlockRatioFee` define a rational number that defines the | ||
| // ref_time to fee mapping. The numbers chosen here are exactly the same as the one from the | ||
| // `WeightToFeePolynomial` that was used before: | ||
| // - The numerator is `currency::CENTS` = 1_000_000_000_000 / 30 / 100 = 333_333_333 | ||
| // - The denominator is `100 * Balance::from(ExtrinsicBaseWeight::get().ref_time())` | ||
| // - which is 100 * 1_000 * 108_157 = 10_815_700_000 | ||
| type WeightToFee = pallet_revive::evm::fees::BlockRatioFee<333333333, 10_815_700_000, Self>; |
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.
WeightToFee was changed to pallet_revive::evm::fees::BlockRatioFee<333333333, 10_815_700_000, Self> with hard-coded numerator/denominator values. This is brittle: if ExtrinsicBaseWeight (or the fee polynomial inputs) change in a future SDK bump, the mapping will silently diverge from the intended economics. Prefer deriving these constants from shared constants (or using the prior WeightToFeePolynomial-based approach) so the mapping updates with upstream changes, or at least centralize these numbers as named consts with a comment pointing to the exact upstream source/version they were taken from.
| type OnChargeTransaction = | ||
| pallet_transaction_payment::FungibleAdapter<Balances, ResolveTo<StakingPot, Balances>>; |
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.
pallet_transaction_payment::Config::OnChargeTransaction changed from a custom fungibles-based adapter (removed in impls.rs) to pallet_transaction_payment::FungibleAdapter<Balances, ResolveTo<StakingPot, Balances>>. This can be a breaking behavior change if Asset Hub Polkadot relied on fungibles credits (e.g., for pallet_asset_conversion_tx_payment integration and paying fees via assets), as the removed adapter explicitly handled fungibles credits and refunds. Verify that asset-based fee payment still works as intended after this change; if it’s still required, keep a fungibles-compatible OnChargeTransaction (or replace it with the new upstream equivalent, if polkadot-sdk now provides one) so ChargeAssetTxPayment/swap adapters aren’t implicitly disabled.
Prefund Ethereum style testnet accounts
This upgrades polkadot-sdk crates to the latest version of
unstable2507. The main change is the following backport: paritytech/polkadot-sdk#10552.This is an important step for upgrading the Kusama AH runtime to the latest version of pallet-revive with: