Remove deprecated leave delegator functionality #2349

nbaztec · 2023-06-14T12:00:59Z

What does it do?

removes deprecated leave delegator functionality

⚠️ Breaking Change ⚠️

❗ schedule_leave_delegators

Removed in favor of batch schedule_revoke_delegation
Replace by extrinsic removed_call_19 that always returns a RemovedCall error.

❗ execute_leave_delegators

Removed in favor of batch execute_delegation_request
Replace by extrinsic removed_call_20 that always returns a RemovedCall error.

❗ cancel_leave_delegators

Removed in favor of batch cancel_delegation_request
Replace by extrinsic removed_call_21 that always returns a RemovedCall error.

❗ execute_delegation_request emits a DelegationRevoked event before a DelegatorLeft event if the last delegation for a delegator is revoked, which was not the case for execute_leave_delegators which would only emit the DelegatorLeft event.

github-actions · 2023-07-11T13:12:41Z

Coverage generated "Tue Aug 8 16:19:31 UTC 2023":
https://d3ifz9vhxc2wtb.cloudfront.net/pulls/2349/html/index.html

Master coverage: 87.39%
Pull coverage:

notlesh · 2023-07-26T21:51:07Z

pallets/parachain-staking/src/lib.rs

-		/// Request to leave the set of delegators. If successful, the caller is scheduled to be
-		/// allowed to exit via a [DelegationAction::Revoke] towards all existing delegations.
-		/// Success forbids future delegation requests until the request is invoked or cancelled.
-		#[pallet::call_index(19)]


One thing that is really important is to help make sure we never reuse these call indices. At a minimum, leaving a comment would help, but I think some test cases would be better.

It might suffice to leave these indices unused for a full runtime upgrade cycle, but also note that other projects which use parachain_staking will have the same potential problem with reuse. The safe choice is clearly to prohibit them from being used again in the future.

To elaborate: in the past we had a problem where an extrinsic got recorded in the scheduler with a specific call index, and the pallet shuffled the call indices around during a runtime upgrade. So the extrinsic was decoded and recorded in one runtime and then executed (with its call index misinterpreted) in the next runtime.

These calls probably wouldn't end up in the scheduler, so maybe that's not a problem. It still is safe and harmless to make sure we avoid them in the future, though...

Yeah I thought about it and even wanted to write a test, but realized that we need to cherry-pick this PR https://github.com/paritytech/substrate/pull/13558/files into substrate (and maybe maintain until it lands in a release) to get this test to actually get the call_indices defined on a pallet. Alternatively we can just always return an error and keep the extrinsic as is.

Shawn had the same suggestion: leave some dummy functions with these call indices which are no-ops. This would probably be the best way to ensure they aren't used.

For the record, he also didn't think it was a big deal :)

I'd actually be against a no-op since this gives the user a false sense of security when they dispatch it. They might just assume a success for whatever they thought might happen with that dispatchable :) An error is more explicit to call for attention in that regard.

However, in scope of this PR do we have any opinions if we should/should not add such a function?

Maybe @librelois has an opinion?

An error is more explicit to call for attention in that regard.

I agree, I think that the best way is to keep theses calls but always return a new error like "RemovedCall"

You think it's also worthwhile to rename the calls to removed_call_{{index}}?

Agree with the above: an explicit failure is good, and making the name obnoxiously obvious about its purpose is good. The whole point is to squat on the call indices.

Thanks updated, please check

fgamundi

Did a quick check to see if there's still some usage of the deprecated calls. Searching in Subscan for DelegatorExitScheduled events in the last year:

Moonbeam: 8 events in the last year, 1 in the last three months
Moonriver: 3 events in the last year, 1 in the last three months
Moonbase: 2 events in the last year (1 by me)

I think we're good to remove them

* remove leave delegator functionality * remove precompile methods * return error for removed call indices

* progress on PoV tests * started precompile migration * finished author precompile tests * bn and callpermit precompiles done * collective precompile done * beginning conviction voting tests * Updated to moonwall4 * fix * added erc20 precompile tests * progress on local asset precompile * Added Local assets tests * modexp tests done * Added proxy tests * progress on proxy tests * finished proxy tests * updated precompile tests * added more precompile tests * progress * added more precompile tests * refactored wormhole tests * added XCM transactor tests * started xcm utils tests * Precompile tests done * done proxy tests * randomness tests * randomness tests * randomness tests * finished randomness tests * added receipt tests * staking tests * more staking tests * more staking * progress * completed staking tests * added subscription tests * added sudo tests * added treasury tests * update moonwall config * added missing conviction tests * txpool progress * tx pool tests done * CI fix * updated prettier CI * editorconfig * pnpm cache * changed CI * fix CI * changes * fix CI * fixed some tests * more test fixes * fixed tests * lint * editor config * pkg fix * updated lockfile * pkg updates * fix import * fix import * archived old-style dev-tests * lint * PR comments * Port ERC20-XCM tests. Add excess gas tests (#2410) * ERC20XCM tests * Add tsx * Moved expect out of xcm helper * Removed unused imports * removed old tests * editorconfig grr * removed smoke tests * Upgraded randomness smoketest * updated randomness comments * Fix conviction MaxVotes (#2401) * Reduce the ConvictionVoting MaxVotes from 512 to 30 * Set MaxVote to 20 * Run toml-sort and update repo reference (#2426) * [MOON-2434] remove deprecated leave delegator functionality (#2349) * remove leave delegator functionality * remove precompile methods * return error for removed call indices * removed scheduleLeaveDelegators calls * de-bun 🫓 the CI * fix * smoke-fix * fixed ci --------- Co-authored-by: Francisco Gamundi <52399794+fgamundi@users.noreply.github.com> Co-authored-by: Alan Sapede <alan.sapede@gmail.com>

* progress on PoV tests * started precompile migration * finished author precompile tests * bn and callpermit precompiles done * collective precompile done * beginning conviction voting tests * Updated to moonwall4 * fix * added erc20 precompile tests * progress on local asset precompile * Added Local assets tests * modexp tests done * Added proxy tests * progress on proxy tests * finished proxy tests * updated precompile tests * added more precompile tests * progress * added more precompile tests * refactored wormhole tests * added XCM transactor tests * started xcm utils tests * Precompile tests done * done proxy tests * randomness tests * randomness tests * randomness tests * finished randomness tests * added receipt tests * staking tests * more staking tests * more staking * progress * completed staking tests * added subscription tests * added sudo tests * added treasury tests * update moonwall config * added missing conviction tests * txpool progress * tx pool tests done * CI fix * updated prettier CI * editorconfig * pnpm cache * changed CI * fix CI * changes * fix CI * fixed some tests * more test fixes * fixed tests * lint * editor config * pkg fix * updated lockfile * pkg updates * fix import * fix import * archived old-style dev-tests * lint * PR comments * Port ERC20-XCM tests. Add excess gas tests (#2410) * ERC20XCM tests * Add tsx * Moved expect out of xcm helper * Removed unused imports * removed old tests * editorconfig grr * begin xcm migration * migrate more xcm tests to moonwall using xcmv3 * removed smoke tests * Upgraded randomness smoketest * updated randomness comments * Fix conviction MaxVotes (#2401) * Reduce the ConvictionVoting MaxVotes from 512 to 30 * Set MaxVote to 20 * Run toml-sort and update repo reference (#2426) * [MOON-2434] remove deprecated leave delegator functionality (#2349) * remove leave delegator functionality * remove precompile methods * return error for removed call indices * removed scheduleLeaveDelegators calls * de-bun 🫓 the CI * fix * migrate hrmp-asset-transfer tests * apply some fixes * migrate test-mock-hrmp-transact-ethereum * small fixes * migrate test-mock-hrmp-transact * migrate more tests to moonwall * migrate test-xcmv3-new-instructions and max weight * create separate folders for v2 and v3 * migrate test-mock-hrmp-transact tests to v3 * migrate mock-hrmp-transact-ethereum tests to v3 * fix network "Any" in tests * fix order in xcmv2 tests * fix order in xcmv3 tests * remove replaceNetworkAny * prettier * fmt * fix orders * remove "Any" type from test-xcm-erc20-v3-filter * use proper GLMR constant * remove cast to any in xcmv2 tests * remove cast to any in xcmv3 tests and re-org * small fixes * add expect messages * refactor registerForeignAsset * fixes in test-mock-dmp-queue * remove BN from AssetMetadata * use proper readContract function * use proper deployContract function * remove BN from requireWeightAtMost * use sovereignAccountOfSibling helper function * remove expectOk --------- Co-authored-by: timbrinded <79199034+timbrinded@users.noreply.github.com> Co-authored-by: Francisco Gamundi <52399794+fgamundi@users.noreply.github.com> Co-authored-by: Alan Sapede <alan.sapede@gmail.com>

nbaztec added 6 commits June 14, 2023 17:28

remove leave delegator functionality

b25a31a

remove precompile methods

b982c25

fix tests

1f231d6

Merge branch 'master' into nish-remove-leave-delegators-deprecated

9b9b423

lint

f1c7399

fix tests

fae3152

nbaztec added D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited breaking Needs to be mentioned in breaking changes B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes labels Jul 11, 2023

fix precompile tests

d96ea02

notlesh reviewed Jul 26, 2023

View reviewed changes

fgamundi reviewed Jul 27, 2023

View reviewed changes

return error for removed call indices

e36eecd

nbaztec requested a review from notlesh August 8, 2023 13:48

nbaztec added 2 commits August 8, 2023 15:49

Merge branch 'master' into nish-remove-leave-delegators-deprecated

dc07606

fmt

8bdf11c

notlesh approved these changes Aug 8, 2023

View reviewed changes

fix const weight warning

64adf65

nbaztec merged commit ad27c07 into master Aug 9, 2023
29 checks passed

nbaztec deleted the nish-remove-leave-delegators-deprecated branch August 9, 2023 13:29

timbrinded added a commit that referenced this pull request Aug 9, 2023

[MOON-2434] remove deprecated leave delegator functionality (#2349)

c9763c3

* remove leave delegator functionality * remove precompile methods * return error for removed call indices

noandrea changed the title ~~[MOON-2434] remove deprecated leave delegator functionality~~ Remove deprecated leave delegator functionality Aug 30, 2023

notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Sep 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove deprecated leave delegator functionality #2349

Remove deprecated leave delegator functionality #2349

nbaztec commented Jun 14, 2023 •

edited

Loading

github-actions bot commented Jul 11, 2023 •

edited

Loading

notlesh Jul 26, 2023

notlesh Jul 26, 2023

nbaztec Jul 27, 2023

notlesh Jul 27, 2023

nbaztec Jul 31, 2023

librelois Aug 2, 2023 •

edited

Loading

nbaztec Aug 7, 2023

notlesh Aug 7, 2023

nbaztec Aug 8, 2023

fgamundi left a comment •

edited

Loading

Remove deprecated leave delegator functionality #2349

Remove deprecated leave delegator functionality #2349

Conversation

nbaztec commented Jun 14, 2023 • edited Loading

What does it do?

github-actions bot commented Jul 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

librelois Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgamundi left a comment • edited Loading

Choose a reason for hiding this comment

nbaztec commented Jun 14, 2023 •

edited

Loading

github-actions bot commented Jul 11, 2023 •

edited

Loading

librelois Aug 2, 2023 •

edited

Loading

fgamundi left a comment •

edited

Loading