Add pallet_revive to Polkadot AssetHub#1050
Conversation
There was a problem hiding this comment.
Found fee-calculation semantic risks (notably proof-size charging), an inconsistency in migration call filtering for the new Revive pallet, and a fragile extrinsic construction workaround introduced by the new UncheckedExtrinsic type; also a couple of test/formatting regressions and Revive config choices that warrant tightening/justification.
Suggestions that couldn't be attached to a specific line
system-parachains/asset-hubs/asset-hub-kusama/src/lib.rs:2779, 2813-2814
Two issues in the updated tests: (1) there is a stray blank line/whitespace-only line in the imports/aliases section ([Line 2779]) which should be removed to avoid churn/noise; (2) the proof-size fee assertion was weakened from assert_eq!(fee, CENTS, "10kb maps to CENT") to only checking non-zero ([Line 2813-2814]). This reduces the test’s ability to detect accidental fee-regression in proof-size pricing. Action: remove the whitespace-only line and replace the weakened assertion with an explicit expected mapping for proof_size under the new WeightToFee (even if the expected value changes, assert that value/range).
system-parachains/asset-hubs/asset-hub-polkadot/src/ah_migration/call_filter.rs:141, 210
Revive is set to OFF in call_allowed_status before migration ([Line 141]) but is included as ON in call_allowed_before_migration ([Line 210]). This is internally inconsistent and makes it unclear whether Revive calls are intended to be permitted pre-migration. Action: decide the intended policy and make both functions agree (and add a short comment for Revive specifically, since this pallet is newly introduced and likely sensitive).
system-parachains/asset-hubs/asset-hub-polkadot/src/staking/mod.rs:559, 568
Extrinsic construction is changed to use <UncheckedExtrinsic as TypeInfo>::Identity::new_transaction/new_bare(...).into() ([Line 559], [Line 568]). This is a fragile coupling to SCALE-info metadata (TypeInfo::Identity) rather than the actual runtime extrinsic API, and it may break if the TypeInfo derive/Identity changes or if UncheckedExtrinsic’s wrapper semantics evolve. Action: introduce an explicit base extrinsic type alias and construct that instead, e.g. type BaseUx = sp_runtime::generic::UncheckedExtrinsic<Address, RuntimeCall, Signature, TxExtension>; BaseUx::new_transaction(...).into(), or add/consume a dedicated constructor provided by the new pallet_revive::evm::runtime::UncheckedExtrinsic wrapper.
|
/cmd fmt |
|
Command "fmt" has started 🚀 See logs here |
|
Command "fmt" has finished ✅ See logs here |
In polkadot-fellows/runtimes#1050 I am unifying the used `WeightToFee` formula to use `BlockRatioFee` everywhere. This type is exported by `pallet_revive` and is independent of any functionality here. However, so far it required the runtime to use `pallet_revive` in order to discover the `Balance` type. This PR removes this constraint so it can be used by all runtimes. Even the ones which do not use `pallet_revive`. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
In polkadot-fellows/runtimes#1050 I am unifying the used `WeightToFee` formula to use `BlockRatioFee` everywhere. This type is exported by `pallet_revive` and is independent of any functionality here. However, so far it required the runtime to use `pallet_revive` in order to discover the `Balance` type. This PR removes this constraint so it can be used by all runtimes. Even the ones which do not use `pallet_revive`. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> (cherry picked from commit 8d6ccfd)
In polkadot-fellows/runtimes#1050 I am unifying the used `WeightToFee` formula to use `BlockRatioFee` everywhere. This type is exported by `pallet_revive` and is independent of any functionality here. However, so far it required the runtime to use `pallet_revive` in order to discover the `Balance` type. This PR removes this constraint so it can be used by all runtimes. Even the ones which do not use `pallet_revive`. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> (cherry picked from commit 8d6ccfd)
|
@claravanstaden This PR changes the weight formula for all system chains. Including bridge hub. This integration/runtime test are now failing: Could it be related to changing weight fees? Can you have a look? |
|
|
Amazing, will this include: paritytech/polkadot-sdk#10476 ? |
Yes. Both kusama and polkadot will get this fix as part of this PR. |
athei
left a comment
There was a problem hiding this comment.
I needed to adjust hard coded weight values due to the weight formula changes. I pinged the people I think should review those changes.
| pub const BridgeHubKusamaBaseXcmFeeInKsms: u128 = 757_124_666; | ||
| pub const BridgeHubKusamaBaseXcmFeeInKsms: u128 = 36_453_538; | ||
|
|
||
| /// Transaction fee that is paid at the Kusama BridgeHub for delivering single inbound message. | ||
| /// (initially was calculated by test `BridgeHubKusama::can_calculate_fee_for_complex_message_delivery_transaction` + `33%`) | ||
| pub const BridgeHubKusamaBaseDeliveryFeeInKsms: u128 = 3_142_112_953; | ||
| pub const BridgeHubKusamaBaseDeliveryFeeInKsms: u128 = 709_970_588; | ||
|
|
||
| /// Transaction fee that is paid at the Kusama BridgeHub for delivering single outbound message confirmation. | ||
| /// (initially was calculated by test `BridgeHubKusama::can_calculate_fee_for_complex_message_confirmation_transaction` + `33%`) | ||
| pub const BridgeHubKusamaBaseConfirmationFeeInKsms: u128 = 575_036_072; | ||
| pub const BridgeHubKusamaBaseConfirmationFeeInKsms: u128 = 181_274_402; |
There was a problem hiding this comment.
@acatangiu @franciscoaguirre The weight formula changed. I adjusted those values in order to make the tests succeed. Please review if it is okay to make this change.
There was a problem hiding this comment.
Why are the number that much lower? There shouldn't be this much of a difference, no?
If we just made things cheaper then I'm fine with it
There was a problem hiding this comment.
Why are the number that much lower?
I think because of in-memory weights: #1025
| pub const BridgeHubPolkadotBaseXcmFeeInDots: Balance = 114_080_750; | ||
| pub const BridgeHubPolkadotBaseXcmFeeInDots: Balance = 5_638_281; | ||
|
|
||
| /// Transaction fee that is paid at the Polkadot BridgeHub for delivering single inbound message. | ||
| /// (initially was calculated by test `BridgeHubPolkadot::can_calculate_fee_for_standalone_message_delivery_transaction` + `33%`) | ||
| pub const BridgeHubPolkadotBaseDeliveryFeeInDots: Balance = 471_317_032; | ||
| pub const BridgeHubPolkadotBaseDeliveryFeeInDots: Balance = 106_495_676; | ||
|
|
||
| /// Transaction fee that is paid at the Polkadot BridgeHub for delivering single outbound message confirmation. | ||
| /// (initially was calculated by test `BridgeHubPolkadot::can_calculate_fee_for_standalone_message_confirmation_transaction` + `33%`) | ||
| pub const BridgeHubPolkadotBaseConfirmationFeeInDots: Balance = 86_267_609; | ||
| pub const BridgeHubPolkadotBaseConfirmationFeeInDots: Balance = 27_027_016; |
There was a problem hiding this comment.
@acatangiu @franciscoaguirre The weight formula changed. I adjusted those values in order to make the tests succeed. Please review if it is okay to make this change.
@bkontur can you please look? |
| type InstantiateOrigin = EnsureSigned<Self::AccountId>; | ||
| type RuntimeHoldReason = RuntimeHoldReason; | ||
| type CodeHashLockupDepositPercent = CodeHashLockupDepositPercent; | ||
| type ChainId = ConstU64<420_420_418>; |
There was a problem hiding this comment.
that's kusama value here isn' it?
There was a problem hiding this comment.
Good catch! I'll need to adjust.
|
|
||
| fn create_transaction(call: RuntimeCall, extension: TxExtension) -> UncheckedExtrinsic { | ||
| UncheckedExtrinsic::new_transaction(call, extension) | ||
| <UncheckedExtrinsic as TypeInfo>::Identity::new_transaction(call, extension).into() |
There was a problem hiding this comment.
Can't guess why but I trust you :D
There was a problem hiding this comment.
Maybe you shouldn't. I just took this from the kusama runtime. I will have a look at this and maybe they both need to be changed so something more elegant.
There was a problem hiding this comment.
Okay I switched to explicitly naming the correct type. Also fixed up the runtime where I copied the code from.
| type MaxEthExtrinsicWeight = MaxEthExtrinsicWeight; | ||
| // Must be set to `false` in a live chain | ||
| type DebugEnabled = ConstBool<false>; | ||
| type GasScale = ConstU32<80_000>; |
There was a problem hiding this comment.
The PRdoc where this is introduced recommends 50k and 100k. What is the rationale here?
There was a problem hiding this comment.
This value depends on the deposit fees, weight fees and decimals of the chain. We need to choose it in a way that the numerical gas values are not higher than on Ethereum. This is because wallets use hard coded values for transfers. See: #1050 (comment)
So the tuning is different for different chains.
| ) { | ||
| // On AH the xcm fee is 26_789_690 and the ED is 3_300_000 | ||
| send_token_from_ethereum_to_asset_hub_with_fee([1; 32], 30_000_000); | ||
| // On AH, ED is 0.1 DOT. Make both the transfer amount (in WETH) and the XCM fee below the ED. |
There was a problem hiding this comment.
Isn't the ED 0.01 DOT?
| Staking: pallet_staking_async = 89, | ||
|
|
||
| // Contracts | ||
| Revive: pallet_revive = 90, |
| pub const BridgeHubKusamaBaseXcmFeeInKsms: u128 = 757_124_666; | ||
| pub const BridgeHubKusamaBaseXcmFeeInKsms: u128 = 36_453_538; | ||
|
|
||
| /// Transaction fee that is paid at the Kusama BridgeHub for delivering single inbound message. | ||
| /// (initially was calculated by test `BridgeHubKusama::can_calculate_fee_for_complex_message_delivery_transaction` + `33%`) | ||
| pub const BridgeHubKusamaBaseDeliveryFeeInKsms: u128 = 3_142_112_953; | ||
| pub const BridgeHubKusamaBaseDeliveryFeeInKsms: u128 = 709_970_588; | ||
|
|
||
| /// Transaction fee that is paid at the Kusama BridgeHub for delivering single outbound message confirmation. | ||
| /// (initially was calculated by test `BridgeHubKusama::can_calculate_fee_for_complex_message_confirmation_transaction` + `33%`) | ||
| pub const BridgeHubKusamaBaseConfirmationFeeInKsms: u128 = 575_036_072; | ||
| pub const BridgeHubKusamaBaseConfirmationFeeInKsms: u128 = 181_274_402; |
There was a problem hiding this comment.
Why are the number that much lower? There shouldn't be this much of a difference, no?
If we just made things cheaper then I'm fine with it
|
/merge |
|
Enabled Available commands
For more information see the documentation |
| type Trader = ( | ||
| UsingComponents< | ||
| WeightToFee, | ||
| WeightToFee<Runtime>, |
There was a problem hiding this comment.
Very ultra nit or just question, why not to use directly KsmWeightToFee?
| WeightToFee<Runtime>, | |
| KsmWeightToFee<Runtime>, |
When somebody will serach for WeightToFee, it could get little bit confusing :)
KsmWeightToFee as WeightToFee
fee::WeightToFee as KsmWeightToFee,
type WeightToFee = KsmWeightToFee<Runtime>;
Basically, we are doing some aliasing, everywhere we import WeightToFee from constants:
use system_parachains_constants::kusama::{currency::*, fee::WeightToFee as KsmWeightToFee};
...
type WeightToFee = KsmWeightToFee<Runtime>;
Maybe, if we rename WeightToFee to KsmWeightToFee in the constants, we won't need to do any aliasing and just use directly KsmWeightToFee.
There was a problem hiding this comment.
I am completely open to whatever people think is the most elegant. I used so much aliasing to keep the code churn down.
In addition to adding the pallet I also:
WeightToFeedefinition and replaced all occurrences with the new oneWeightToFeedefinitionWeightToFeewith new definition required forpallet_reviveThe only difference to the Kusama config is: