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

chore: try-runtime variation check on new thresholds #1317

Merged
merged 1 commit into from
Aug 9, 2024

Conversation

ipapandinas
Copy link
Contributor

@ipapandinas ipapandinas commented Aug 7, 2024

Pull Request Summary

Adds try-runtime sanity checks to ensure new tier threshold amounts are within a given variation range as suggested in this comment. The allowed percentage can be customized using the ThresholdVariationPercentage parameter.

Variations allowed by runtime:

  • Astar - 10%
  • Shiden - 10%
  • Shibuya - 150%

The variation tolerated is significant on Shibuya given the static tier params percentages configured. However, I used the same calculation as Astar/Shiden: a percentage of the total Shibuya issuance when dApp Staking v3 was launched, so 147M $SBY. Here are the expected amounts when testing with try-runtime in the post_upgrade hook:

[2024-08-07T12:21:26Z WARN  pallet_dapp_staking_v3::migration::v8] old_threshold_amounts: BoundedVec([2500000000000000000000, 1056338028169014084000, 563380281690140844800, 200000000000000000000], 4)
[2024-08-07T12:21:26Z WARN  pallet_dapp_staking_v3::migration::v8] expected_new_threshold_amounts: BoundedVec([3879187284712957159095, 2281874873360563034762, 867755233531481717028, 319462482270478824867], 4)

Check list

  • added or updated unit tests
  • updated Astar official documentation
  • added OnRuntimeUpgrade hook for precompile revert code registration
  • added benchmarks & weights for any modified runtime logics.

@ipapandinas ipapandinas added dapps-staking Dapps Staking tests If the PR/issue is related to tests, like xcm-simulator tests, rpc-tests etc. labels Aug 7, 2024
@ipapandinas ipapandinas changed the title chore: try-runtime check on new threshold variations chore: try-runtime variation check on new thresholds Aug 7, 2024
Copy link
Member

@Dinonard Dinonard left a comment

Choose a reason for hiding this comment

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

Looks good, just a comment about Percentage usage.

You can replace u32 with Percent::from_percent, and get rid of the custom arithmetic.

impl<
T: Config,
TierThresholds: Get<[TierThreshold; 4]>,
ThresholdVariationPercentage: Get<u32>,
Copy link
Member

Choose a reason for hiding this comment

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

Percent would make more sense here (or Permill, Perbill).
That way you have guarantee that's it's not larger than 1 or 100%.

Comment on lines +239 to +241
let lower_bound = old_amount
.saturating_mul(100u32.saturating_sub(variation_percentage).into())
.saturating_div(100u32.into());
let upper_bound = old_amount
.saturating_mul(100u32.saturating_add(variation_percentage).into())
.saturating_div(100u32.into());
Copy link
Member

Choose a reason for hiding this comment

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

I'd definitely suggest to use Percent here.

@ipapandinas
Copy link
Contributor Author

ipapandinas commented Aug 8, 2024

You can replace u32 with Percent::from_percent, and get rid of the custom arithmetic.

I thought about it but I am expecting a variation of +108% for Tier 2 in Shibuya.. 🥲 2,282 SBY (new) vs 1,056 SVY (current).

It is explained in PR bio:

The variation tolerated is significant on Shibuya given the static tier params percentages configured. However, I used the same calculation as Astar/Shiden: a percentage of the total Shibuya issuance when dApp Staking v3 was launched, so 147M $SBY. Here are the expected amounts when testing with try-runtime in the post_upgrade hook:

[2024-08-07T12:21:26Z WARN  pallet_dapp_staking_v3::migration::v8] old_threshold_amounts: BoundedVec([2500000000000000000000, 1056338028169014084000, 563380281690140844800, 200000000000000000000], 4)
[2024-08-07T12:21:26Z WARN  pallet_dapp_staking_v3::migration::v8] expected_new_threshold_amounts: BoundedVec([3879187284712957159095, 2281874873360563034762, 867755233531481717028, 319462482270478824867], 4)

The transformation of TVL amounts into dynamic percentages is not well suited to Shibuya; the percentages seem very low with the same calculation approach as for Astar and Shiden.

I can readjust static tier percentages to match current threshold amounts and use Percent::from_percent as you suggest (but percentages will be even lower) OR preserve a 150% tolerance with the custom arithmetic, WDYT @Dinonard ?

@Dinonard
Copy link
Member

Dinonard commented Aug 9, 2024

I've read that it's 15% for Shibuya lol.
But it makes sense that the difference is so big because we minted huge amount of SBY tokens few months ago.

You can keep the code as it is, it was more of a suggestion than req.for change.

I'd opt for Percent myself 🙂 , and just skip the Shibuya check altogether since it doesn't really make sense anyways - you need to custom set a large number in order for it to pass, but it doesn't bring any real testing value.

Dinonard
Dinonard previously approved these changes Aug 9, 2024
Copy link

github-actions bot commented Aug 9, 2024

Code Coverage

Package Line Rate Branch Rate Health
pallets/dynamic-evm-base-fee/src 89% 0%
precompiles/sr25519/src 64% 0%
pallets/dapp-staking-migration/src 0% 0%
precompiles/dapp-staking-v3/src 90% 0%
precompiles/xcm/src 71% 0%
pallets/unified-accounts/src 86% 0%
pallets/dapp-staking-v3/rpc/runtime-api/src 0% 0%
precompiles/dispatch-lockdrop/src 86% 0%
chain-extensions/pallet-assets/src 56% 0%
pallets/collator-selection/src 92% 0%
pallets/dapp-staking-v3/src 84% 0%
pallets/astar-xcm-benchmarks/src/fungible 100% 0%
pallets/dapp-staking-v3/src/benchmarking 98% 0%
pallets/astar-xcm-benchmarks/src 86% 0%
precompiles/assets-erc20/src 78% 0%
precompiles/dapp-staking-v3/src/test 0% 0%
chain-extensions/types/unified-accounts/src 0% 0%
primitives/src/xcm 65% 0%
pallets/dapp-staking-v3/src/test 0% 0%
chain-extensions/types/assets/src 0% 0%
pallets/xc-asset-config/src 50% 0%
pallets/static-price-provider/src 85% 0%
precompiles/substrate-ecdsa/src 74% 0%
pallets/astar-xcm-benchmarks/src/generic 100% 0%
pallets/price-aggregator/src 82% 0%
pallets/collective-proxy/src 86% 0%
precompiles/unified-accounts/src 100% 0%
primitives/src 59% 0%
pallets/ethereum-checked/src 74% 0%
pallets/inflation/src 93% 0%
chain-extensions/unified-accounts/src 0% 0%
Summary 77% (3565 / 4627) 0% (0 / 0)

Minimum allowed line rate is 50%

@ipapandinas ipapandinas merged commit 6609050 into master Aug 9, 2024
8 checks passed
@ipapandinas ipapandinas deleted the chore/minor-try-runtime-check branch August 9, 2024 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dapps-staking Dapps Staking tests If the PR/issue is related to tests, like xcm-simulator tests, rpc-tests etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants