-
Notifications
You must be signed in to change notification settings - Fork 9
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
refactor(mainnet): monetary config #440
refactor(mainnet): monetary config #440
Conversation
d68af12
to
937ca11
Compare
type ExistentialDeposit = ExistentialDeposit; | ||
type FreezeIdentifier = RuntimeFreezeReason; | ||
type MaxFreezes = VariantCountOf<RuntimeFreezeReason>; | ||
type MaxLocks = ConstU32<0>; |
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.
Locks are deprecated.
type FreezeIdentifier = RuntimeFreezeReason; | ||
type MaxFreezes = VariantCountOf<RuntimeFreezeReason>; | ||
type MaxLocks = ConstU32<0>; | ||
type MaxReserves = ConstU32<0>; |
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.
Reserves are deprecated.
type FeeMultiplierUpdate = SlowAdjustingFeeUpdate<Self>; | ||
type LengthToFee = ConstantMultiplier<Balance, TransactionByteFee>; | ||
type OnChargeTransaction = | ||
pallet_transaction_payment::FungibleAdapter<Balances, ResolveTo<SudoAddress, 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.
Needs to align with the result of [sc-2260]
7359e7a
to
9594a3b
Compare
8792be4
to
e209cce
Compare
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## al3mart/refactor-mainnet-config #440 +/- ##
==================================================================
Coverage ? 72.55%
==================================================================
Files ? 77
Lines ? 14472
Branches ? 14472
==================================================================
Hits ? 10500
Misses ? 3703
Partials ? 269
|
@@ -131,7 +131,7 @@ impl pallet_transaction_payment::Config for Runtime { | |||
type FeeMultiplierUpdate = SlowAdjustingFeeUpdate<Self>; | |||
type LengthToFee = ConstantMultiplier<Balance, TransactionByteFee>; | |||
type OnChargeTransaction = | |||
pallet_transaction_payment::FungibleAdapter<Balances, ResolveTo<SudoAddress, Balances>>; | |||
pallet_transaction_payment::FungibleAdapter<Balances, ResolveTo<TreasuryAccount, 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.
Still needs aligning with [sc-2260] but allowed me to remove sudo completely from here.
dc7df88
to
f49c3c6
Compare
value: UNIT + (existential_deposit / 2), | ||
}); | ||
Balances::set_balance(&who, existential_deposit + UNIT); | ||
Balances::set_balance(&TreasuryAccount::get(), UNIT); |
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.
TreasuryAccount existence needs to be covered before it starts receiving dust amounts. Otherwise dust won't be resolved into the account.
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.
Approving! Nice.
I left two comments you could commit directly in GitHub that use the ED value instead of UNIT for the dust test. Not a blocker though as it's a small detail.
value: UNIT + (existential_deposit / 2), | ||
}); | ||
Balances::set_balance(&who, existential_deposit + UNIT); | ||
Balances::set_balance(&TreasuryAccount::get(), UNIT); |
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.
more accurate to use the ED value, right?
Balances::set_balance(&TreasuryAccount::get(), UNIT); | |
Balances::set_balance(&TreasuryAccount::get(), existential_deposit); |
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 like it!
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.
Resolved in 2fdfee7 as I spotted an import that could be removed.
// Treasury balance equals its initial balance + the dusted amount from `who`. | ||
assert_eq!( | ||
Balances::free_balance(&TreasuryAccount::get()), | ||
UNIT + existential_deposit / 2 |
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.
UNIT + existential_deposit / 2 | |
existential_deposit + existential_deposit / 2 |
if you agree with using ED value instead of UNIT for the initial treasury balance.
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.
As above.
fn transaction_payment_weight_to_fee_as_expected() { | ||
let arb_weight = Weight::from_parts(126_142_001, 123_456); | ||
let no_proof_size_weight = Weight::from_parts(126_142_001, 0); | ||
let weights: [Weight; 4] = [ |
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.
Could be a list of the actual weights of some of the extrinsics from the pallets in the runtime, would make for a better test.
Can be updated after we run benchmarks..
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.
LGTM!
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.
Super cool PR, nice to go over the details here again :)
Left a few nitpicks but overall looking goood!
SlowAdjustingFeeUpdate, System, VariantCountOf, EXISTENTIAL_DEPOSIT, | ||
}; | ||
|
||
pub const fn deposit(items: u32, bytes: u32) -> Balance { |
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.
Missing docs for pub
functions
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.
} | ||
|
||
/// Constants related to Polkadot fee payment. | ||
/// Source: https://github.com/polkadot-fellows/runtimes/blob/main/system-parachains/constants/src/polkadot.rs#L65C47-L65C58 |
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.
Doesn't have to be included in docs tmo, serves more as a reference for code maintainers / readers:
/// Source: https://github.com/polkadot-fellows/runtimes/blob/main/system-parachains/constants/src/polkadot.rs#L65C47-L65C58 | |
// Source: https://github.com/polkadot-fellows/runtimes/blob/main/system-parachains/constants/src/polkadot.rs#L65C47-L65C58 |
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.
pub const CENTS: Balance = MILLI_UNIT * 10; // 100_000_000 | ||
pub const MILLI_CENTS: Balance = CENTS / 1_000; // 100_000 | ||
|
||
/// Cost of every transaction byte at Polkadot system parachains. |
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.
Don't understand this sentence and we are not a system parachain.
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.
When calculating the cost of a transaction this price will be multiplied by the length (in #bytes) of the transaction in question.
Removed the system chain reference. The price is aligned with the one system chains charge.
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.
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.
When calculating the cost of a transaction this price will be multiplied by the length (in #bytes) of the transaction in question.
very informative and perfect for the docs of this type
|
||
/// Cost of every transaction byte at Polkadot system parachains. | ||
/// | ||
/// It is the Relay Chain (Polkadot) `TransactionByteFee` / 20. |
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 don't find this adding value as we are using MILLI_CENTS and there is no correlation provided with TransactionByteFee. I also don't understand what this means, what is the TransactionByteFee and why is it 20 times smaller?
It doesn't align with line 126.
Can we also remove the public docs from it, it is more a detail for code mantainers / readers.
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.
Good catch! Updated that comment.
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.
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.
One final check on the TRANSACTION_BYTE_FEE
|
||
parameter_types! { | ||
/// Relay Chain `TransactionByteFee` / 20. | ||
pub const TransactionByteFee: Balance = fee::TRANSACTION_BYTE_FEE; |
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 still don't understand, we are already dividing this constant by 2 and saying the exact same here (both types have the comment of the relay chains value divided by 20). Are not doing what the comment says twice?
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 a comment that doesn't talk about the cost but how TransactionByteFee
is relevant can be more clear ?
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.
We are not doing anything twice. The comment was just a heads up that the value equals the relay's divided by 20.
I've removed this comment.
215f68b
to
f426357
Compare
rebased after merning proxy config changes into the base branch. |
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.
LGTM, well done!
remove extra clone
changes after rebase more changes after reabse
14161f2
to
7f2d4c1
Compare
* refactor(mainnet): pallet_balances & fee constants * test(mainnet): add pallet_balances tests * refactor(mainnet): add pallet_transaction_payment to monetary config * test(mainnet): add transaction_payment tests * fix(mainnet): rebase fixes * fix(monetary): don't burn dust * refactor(monetary): use treasury account instead of sudo remove extra clone * refactor(monetary): use existential_deposit instead of UNIT in dust test * refactor(monetary): Don't use DOLLARS & CENTS is defined in terms of UNITS * fix(monetary): missing doc for fn deposit * docs(monetary): remove src from docs * docs(monetary): update TransactionByteFee comments * refactor(monetary): reorg tests into modules * style(monetary): remove confussing comment * style(monetar): remove prefixes from tests changes after rebase more changes after reabse
* refactor(mainnet): pallet_balances & fee constants * test(mainnet): add pallet_balances tests * refactor(mainnet): add pallet_transaction_payment to monetary config * test(mainnet): add transaction_payment tests * fix(mainnet): rebase fixes * fix(monetary): don't burn dust * refactor(monetary): use treasury account instead of sudo remove extra clone * refactor(monetary): use existential_deposit instead of UNIT in dust test * refactor(monetary): Don't use DOLLARS & CENTS is defined in terms of UNITS * fix(monetary): missing doc for fn deposit * docs(monetary): remove src from docs * docs(monetary): update TransactionByteFee comments * refactor(monetary): reorg tests into modules * style(monetary): remove confussing comment * style(monetar): remove prefixes from tests changes after rebase more changes after reabse
* refactor(mainnet): pallet_balances & fee constants * test(mainnet): add pallet_balances tests * refactor(mainnet): add pallet_transaction_payment to monetary config * test(mainnet): add transaction_payment tests * fix(mainnet): rebase fixes * fix(monetary): don't burn dust * refactor(monetary): use treasury account instead of sudo remove extra clone * refactor(monetary): use existential_deposit instead of UNIT in dust test * refactor(monetary): Don't use DOLLARS & CENTS is defined in terms of UNITS * fix(monetary): missing doc for fn deposit * docs(monetary): remove src from docs * docs(monetary): update TransactionByteFee comments * refactor(monetary): reorg tests into modules * style(monetary): remove confussing comment * style(monetar): remove prefixes from tests changes after rebase more changes after reabse
* refactor(mainnet): add governance & pallet_sudo tests (#443) * refactor(mainnet): add governance & pallet_sudo tests * refactor(sudo): sudo account defined in governance & unit test fix(governance): make governance mod public & better sudo declaration * refactor(mainnet): utility config (#438) * refactor(mainnet): add pallet_multisig to utility * test(mainnet): add pallet_multisig tests * refactor(mainnet): add pallet_utility to utility * test(mainnet): add pallet_utility tests * refactor(mainnet): add pallet_preimage to utility * test(mainnet): add pallet_preimage tests * refactor(mainnet): add pallet_scheduler to utility * test(mainnet): add pallet_scheduler tests * refactor(mainnet): pallet_proxy config & tests (#441) * refactor(mainnet): pallet_proxy config & tests * style(proxy): tests follow config order * refactor(proxy): remove asset related ProxyTypes * refactor(proxy): update ProxyDepositBase price * refactor(proxy): revert d3bfc6c and redefine ProxyType for mainnet typo * fix(proxy): amend proxy_has_deposit_base test * docs(proxy): Clarify new ProxyType definition * docs(proxy): add is_superset comments * test(proxy): clarify deposit byte size lenght * refactor(mainnet): system config (#442) * refactor(mainnet): add frame_system to system * test(mainnet): add pallet_system test * fix(mainnet): rebase fixes * fix(system): bring missing types in scope * refactor(system): add cumulus_parachain_system to system config module * test(system): add cumulus_parachain_system tests * refactor(system): add parachain_info into system config module * refactor(system): add pallet_timestamp into system config module * style(system): add ConsensusHook comment * refactor(system): separate tests by module * docs(system): document public config modules * fix(system): allow unused_imports to remove Executive warning * refactor(system): reintroduce FilteredCalls * style(system): get rid of test prefixes * style(system): relocate max_block_weigth comment * test(system): check for Hash type config * docs(system): amend outdated comments * Update runtime/mainnet/src/config/mod.rs Co-authored-by: Daan van der Plas <[email protected]> * Update runtime/mainnet/src/config/mod.rs Co-authored-by: Daan van der Plas <[email protected]> * Update runtime/mainnet/src/config/mod.rs Co-authored-by: Daan van der Plas <[email protected]> * Update runtime/mainnet/src/config/mod.rs Co-authored-by: Daan van der Plas <[email protected]> --------- Co-authored-by: Daan van der Plas <[email protected]> * refactor(mainnet): monetary config (#440) * refactor(mainnet): pallet_balances & fee constants * test(mainnet): add pallet_balances tests * refactor(mainnet): add pallet_transaction_payment to monetary config * test(mainnet): add transaction_payment tests * fix(mainnet): rebase fixes * fix(monetary): don't burn dust * refactor(monetary): use treasury account instead of sudo remove extra clone * refactor(monetary): use existential_deposit instead of UNIT in dust test * refactor(monetary): Don't use DOLLARS & CENTS is defined in terms of UNITS * fix(monetary): missing doc for fn deposit * docs(monetary): remove src from docs * docs(monetary): update TransactionByteFee comments * refactor(monetary): reorg tests into modules * style(monetary): remove confussing comment * style(monetar): remove prefixes from tests changes after rebase more changes after reabse * refactor(mainnet): collation config (#439) * refactor(mainnet): add pallet_authorship to collation * test(mainnet): add pallet_auhtorship test * refactor(mainnet): add pallet_aura to collation * test(mainnet): add pallet_aura tests * refactor(mainnet): add collator_selection to collation * test(mainnet): add collator_selection tests * refactor(mainnet): add pallet_session & aura_ext to collation * test(mainnet): add pallet_session tests * chore(aura): MaxAuthorities matches max possible blocks produced * remove unnused imports * style(collation): order tests following pallet config * style(collation): remove unnecessary qualifications * style(collation): max_authorities_is_3600 * style(collation): reorg definition in session_keys_provided_by_aura test * style(collation): better Period comment * style(collation): clarify session::SessionHandler comment * refactor(collation): separate tests in modules by pallet * style(collation): Explain SessionHandler config fixes after rebase * style(collation): remove prefixes from tests * style(collation): Period comment was not helpful changes after rebase * style(utility): better test structure style(utility): fmt rebase fmt fmt * refactor(mainnet): xcm config (#455) * refactor(mainnet): rebase changes * chore(mainnet): update Cargo.lock * refactor(mainnet): utility config (#438) * refactor(mainnet): add pallet_multisig to utility * test(mainnet): add pallet_multisig tests * refactor(mainnet): add pallet_utility to utility * test(mainnet): add pallet_utility tests * refactor(mainnet): add pallet_preimage to utility * test(mainnet): add pallet_preimage tests * refactor(mainnet): add pallet_scheduler to utility * test(mainnet): add pallet_scheduler tests * refactor(mainnet): system config (#442) * refactor(mainnet): add frame_system to system * test(mainnet): add pallet_system test * fix(mainnet): rebase fixes * fix(system): bring missing types in scope * refactor(system): add cumulus_parachain_system to system config module * test(system): add cumulus_parachain_system tests * refactor(system): add parachain_info into system config module * refactor(system): add pallet_timestamp into system config module * style(system): add ConsensusHook comment * refactor(system): separate tests by module * docs(system): document public config modules * fix(system): allow unused_imports to remove Executive warning * refactor(system): reintroduce FilteredCalls * style(system): get rid of test prefixes * style(system): relocate max_block_weigth comment * test(system): check for Hash type config * docs(system): amend outdated comments * Update runtime/mainnet/src/config/mod.rs Co-authored-by: Daan van der Plas <[email protected]> * Update runtime/mainnet/src/config/mod.rs Co-authored-by: Daan van der Plas <[email protected]> * Update runtime/mainnet/src/config/mod.rs Co-authored-by: Daan van der Plas <[email protected]> * Update runtime/mainnet/src/config/mod.rs Co-authored-by: Daan van der Plas <[email protected]> --------- Co-authored-by: Daan van der Plas <[email protected]> * refactor(mainnet): collation config (#439) * refactor(mainnet): add pallet_authorship to collation * test(mainnet): add pallet_auhtorship test * refactor(mainnet): add pallet_aura to collation * test(mainnet): add pallet_aura tests * refactor(mainnet): add collator_selection to collation * test(mainnet): add collator_selection tests * refactor(mainnet): add pallet_session & aura_ext to collation * test(mainnet): add pallet_session tests * chore(aura): MaxAuthorities matches max possible blocks produced * remove unnused imports * style(collation): order tests following pallet config * style(collation): remove unnecessary qualifications * style(collation): max_authorities_is_3600 * style(collation): reorg definition in session_keys_provided_by_aura test * style(collation): better Period comment * style(collation): clarify session::SessionHandler comment * refactor(collation): separate tests in modules by pallet * style(collation): Explain SessionHandler config fixes after rebase * style(collation): remove prefixes from tests * style(collation): Period comment was not helpful changes after rebase * refactor(xcm): add pallet_message_queue into xcm config module * test(xcm): add message_queue tests * test(xcm): test location, transactor and barrier type definitions * refactor(xcm): include pallet_xcmp_queue into xcm config module * test(xcm): add pallet_xcmp_queue configuration tests * test(xcm): add xcm_executor configuration tests * test(xcm): adding pallet_xcm config tests * fix(xcm): typo fix(xcm): XcmReserveTransferFilter is Everything * refactor(xcm): use unitype for MaxRemoteLockConsumers * style(xcm): tests separated in modules * style(xcm): better naming for XcmTeleportFilter test * style(xcm): test naming and order * docs(xcm): clarifying comments for specific xcmp_queueu types * style(xcm): typo * style(xcm): better name for reserves tests * refactor(xcm): Use `NativeAssetFrom<T>` as `XcmReserveTransferFilter` (#459) * refactor(xcm): Use NativeAssetFrom<T> as XcmReserveTransferFilter style:fmt * fix(xcm): NativeAssetFrom<AssetHub> filters everything but DOT from AH * fix(xcm): XcmReserveTransferFilter filters only by asset fix changes after rebase fmt * fix(xcm): remove waived locations (#456) * fix(xcm): no locations are waived * test(xcm): improved waived_location test changes after rebase * style(xcm): clarify that we are not waiving fees for any location * fix(xcm): charge delivery fees (#457) * resolve rebase conflicts * fix(xcm): reserve_transfer_native_asset_from_para_to_system_para accounts for delivery fees * fix(integration-tests): handle delivery_fees only with mainnet feature on fmt after rebase fmt changes after rebase fmt fmt * test(xcm): address feedback from #459 typos AssetFilter * style(xcm): locate reserve tests within executor lint * fix(integration-test): improve tests & remove feature gated compilation unnecessary Fungibles * refactor: integration tests mainnet xcm (#466) * refactor: integration tests mainnet xcm * fix: remove unused ED rebase fmt fmt * Update runtime/mainnet/src/config/xcm.rs Co-authored-by: Daan van der Plas <[email protected]> * refactor(xcm): use TreasuryAccount from monetary * chore(xcm): unused imports * chore(xcm): amend test modules * chore(integration-tests): amend reserve_transfer_native_asset_from_para_to_system * chore(xcm): divive reserve filter test in two * chore(xcm): fmt * style(integration-tests): fix unwanted indentation * test(xcm): more focused unit tests * Update runtime/mainnet/src/config/xcm.rs Co-authored-by: Daan van der Plas <[email protected]> * Update runtime/mainnet/src/config/xcm.rs Co-authored-by: Daan van der Plas <[email protected]> * fix(test): amend suggestions --------- Co-authored-by: Daan van der Plas <[email protected]> * feat(mainnet): introduce assets (#465) * refactor(mainnet): include assets fmt refactor(collator): not necessary import * test(assets): add nfts config unit tests * refactor(proxy): Add assets & smart contract proxy type * refactor(assets): implement nfts runtime api * fix(pallet-nfts-api): workspace linters are not defined * docs(assets): clarify deposits * chore(proxy): improve usage of references * docs(assets): update src reference * chore(assets): update MetadataDepositBase * docs(assets): remove TODOs * refactor(proxy): revert SmartContract ProxyType * revert: e614927 * style(Cargo): revert format changes on dep comments * chore(assets): apply feedback to tests * chore(assets): removed unused type * revert: fb92342 This reverts commit fb92342. * docs(assets): better comments * chore(assets): lower nfts deposit costs * chore(assets): reduced deposit cost for AssetAccountDeposit * chore(assets): use SDK's pallet_nfts * chore(assets): better deposits & fmt Cargo * chore(assets): better comments * chore(assets): more explicit comments for deposits * chore(assets): provide src for NftsCollectionDeposit deposit * chore(assets): apply feedback * chore(monetary): clean unnecessary comments * chore(utility): make deposit calc explicit. (#469) chore(utility): explicit deposit justification rebase fixes * chore(proxy): reintroduce Asset related proxy types * syle(proxy): Clarify proxy test comments --------- Co-authored-by: Daan van der Plas <[email protected]>
Creates monetary config module including:
pallet_balances
pallet_transaction_payment
fee
constantsNotable configuration items are:
Mainnet to use its own deposit function different from the one defined in
pop_runtime_common
being used in devnet and testnet. It aligns the deposit pricing with system chains in Polkadot.Maps
ExtrinsicBaseWeight::get().ref_time()
to 1/200 CENT.Maps 20kb proof to 1 CENT.
1_000_000_000
units.pallet_balances
Configured this way as these types are deprecated: paritytech/substrate#12951
pallet_transaction_payment
OnChargeTransaction
needs to align with the result of [sc-2260][sc-2205]