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

Fix account call for freeze/lock inconsistency #1276

Merged
merged 6 commits into from
Jun 24, 2024

Conversation

Dinonard
Copy link
Member

@Dinonard Dinonard commented Jun 21, 2024

Pull Request Summary

Provide an approach to fix the account within an incosistent freeze state.

The approach is as simple as possible, reusing the claim_unlocked logic to clear unlocking chunks which are forcefully set to be immediately claimable.

@Dinonard Dinonard marked this pull request as ready for review June 24, 2024 04:34
@Dinonard Dinonard added the runtime This PR/Issue is related to the topic “runtime”. label Jun 24, 2024
ashutoshvarma
ashutoshvarma previously approved these changes Jun 24, 2024
Copy link
Member

@ashutoshvarma ashutoshvarma left a comment

Choose a reason for hiding this comment

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

LGTM, just some nitpicks

@@ -386,6 +386,8 @@ pub mod pallet {
NoExpiredEntries,
/// Force call is not allowed in production.
ForceNotAllowed,
/// Account doesn't have the freeze inconsistency
InvalidAccount, // TODO: can be removed after call `fix_account` is removed
Copy link
Member

Choose a reason for hiding this comment

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

nit: can be better named, AccountNotInconsistent ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there's the need for this. If account is inconsistent then there's noop tx paying just fee

Copy link
Member Author

Choose a reason for hiding this comment

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

@ashutoshvarma sure. Believe it or not, that was the first name I choose, but decided to go with a more generic one later xD.

@ermalkaleci
I added it to make testing more clear.
It will be removed later anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough

// 2. Execute the unlock call, clearing all of the unlocking chunks.
let result = Self::internal_claim_unlocked(account);

// 3. Adjust consumed weight to consume the max possible weight (as defined in the weight macro).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// 3. Adjust consumed weight to consume the max possible weight (as defined in the weight macro).
// 3. Adjust consumed weight to consume the max possible weight (as defined in `T::WeightInfo::claim_unlocked()`).

nit: internal_claim_unlocked() has no weights macro, but use weights from claim_unlocked().
I was confused at first

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean the macro above the call: T::WeightInfo::claim_unlocked() + const.

Copy link

Code Coverage

Package Line Rate Branch Rate Health
primitives/src/xcm 64% 0%
pallets/dapp-staking-migration/src 0% 0%
pallets/dapp-staking-v3/rpc/runtime-api/src 0% 0%
pallets/dynamic-evm-base-fee/src 92% 0%
pallets/oracle-benchmarks/src 0% 0%
pallets/dapp-staking-v3/src 90% 0%
pallets/astar-xcm-benchmarks/src/fungible 100% 0%
pallets/unified-accounts/src 86% 0%
pallets/collator-selection/src 92% 0%
pallets/ethereum-checked/src 79% 0%
chain-extensions/pallet-assets/src 56% 0%
pallets/astar-xcm-benchmarks/src 88% 0%
precompiles/xvm/src 75% 0%
primitives/src 62% 0%
precompiles/dispatch-lockdrop/src 86% 0%
pallets/xvm/src 54% 0%
precompiles/sr25519/src 64% 0%
chain-extensions/unified-accounts/src 0% 0%
precompiles/substrate-ecdsa/src 74% 0%
precompiles/unified-accounts/src 100% 0%
pallets/dapp-staking-v3/src/benchmarking 98% 0%
chain-extensions/types/xvm/src 0% 0%
pallets/price-aggregator/src 72% 0%
precompiles/assets-erc20/src 81% 0%
pallets/dapp-staking-v3/src/test 0% 0%
pallets/astar-xcm-benchmarks/src/generic 100% 0%
chain-extensions/types/unified-accounts/src 0% 0%
chain-extensions/xvm/src 0% 0%
pallets/xc-asset-config/src 64% 0%
pallets/static-price-provider/src 52% 0%
chain-extensions/types/assets/src 0% 0%
pallets/inflation/src 83% 0%
precompiles/dapp-staking-v3/src 90% 0%
precompiles/dapp-staking-v3/src/test 0% 0%
precompiles/xcm/src 73% 0%
Summary 77% (3662 / 4735) 0% (0 / 0)

Minimum allowed line rate is 50%

@Dinonard Dinonard merged commit 2f9d536 into master Jun 24, 2024
8 checks passed
@Dinonard Dinonard deleted the fix/freeze-lock-inconsistency-fix-call branch June 24, 2024 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants