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

[AHM] Poke deposits: Indices pallet #7587

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

UtkarshBhardwaj007
Copy link
Contributor

@UtkarshBhardwaj007 UtkarshBhardwaj007 commented Feb 16, 2025

Description

Review Notes

  • Added a new extrinsic reconsider in pallet-indices.
  • Added a new event DepositReconsidered to be emitted upon a successful call of the extrinsic.
  • Although the immediate use of the extrinsic will be to give back some of the deposit after the AH-migration, the extrinsic is written such that it can work if the deposit decreases or increases (both).
  • The call to the extrinsic would be free if an actual adjustment is made to the deposit and paid otherwise.
  • Added tests to test all scenarios.
  • Added a benchmark to test the "worst case" (maximum compute) flow of the extrinsic which is when the deposit amount is updated to a new value.

TO-DOs

  • Run CI cmd bot to benchmark

@UtkarshBhardwaj007 UtkarshBhardwaj007 requested a review from a team as a code owner February 16, 2025 23:47
@UtkarshBhardwaj007 UtkarshBhardwaj007 added T1-FRAME This PR/Issue is related to core FRAME, the framework. T2-pallets This PR/Issue is related to a particular pallet. labels Feb 16, 2025
@kianenigma kianenigma changed the title Poke deposits [AHM] Poke deposits: Indices pallet Feb 17, 2025
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Mainly unsure about Permanent, otherwise looks good.

@UtkarshBhardwaj007
Copy link
Contributor Author

UtkarshBhardwaj007 commented Feb 17, 2025

Mainly unsure about Permanent, otherwise looks good.

From what I understand from the code, the freeze extrinsic slashes the deposit here before marking an index as permanent meaning there is no deposit left to reconsider. @kianenigma

@github-actions github-actions bot deleted a comment from UtkarshBhardwaj007 Feb 17, 2025
@github-actions github-actions bot deleted a comment from UtkarshBhardwaj007 Feb 17, 2025
@github-actions github-actions bot deleted a comment from UtkarshBhardwaj007 Feb 17, 2025
@github-actions github-actions bot deleted a comment from UtkarshBhardwaj007 Feb 17, 2025
@paritytech paritytech deleted a comment from github-actions bot Feb 17, 2025
@paritytech paritytech deleted a comment from github-actions bot Feb 17, 2025
@github-actions github-actions bot deleted a comment from UtkarshBhardwaj007 Feb 18, 2025
@github-actions github-actions bot deleted a comment from UtkarshBhardwaj007 Feb 18, 2025
@muharem muharem self-requested a review February 18, 2025 14:03
prdoc/pr_7587.prdoc Outdated Show resolved Hide resolved
Copy link
Contributor

@muharem muharem left a comment

Choose a reason for hiding this comment

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

just minor comments

substrate/frame/indices/src/tests.rs Outdated Show resolved Hide resolved
substrate/frame/indices/src/benchmarking.rs Outdated Show resolved Hide resolved
/// - `O(1)`.
#[pallet::call_index(5)]
#[pallet::weight(T::WeightInfo::reconsider())]
pub fn reconsider(
Copy link
Contributor

Choose a reason for hiding this comment

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

not because of my own taste, but for the sake of consistency. how about poke_deposit?
we already have for the same purpose poke_deposit in some pallet, and reconsider is the new naming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based this off of @kianenigma 's comment here: #5591 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

lets wait for @kianenigma opinion. if Kian wants to keep the reconsider anyway, it's also fine by me.

substrate/frame/indices/src/benchmarking.rs Outdated Show resolved Hide resolved
substrate/frame/indices/src/benchmarking.rs Outdated Show resolved Hide resolved
substrate/frame/indices/src/lib.rs Outdated Show resolved Hide resolved
@paritytech-review-bot paritytech-review-bot bot requested a review from a team February 18, 2025 15:50
} else if new_amount < old_amount {
// Need to unreserve some
let excess = old_amount.saturating_sub(new_amount);
T::Currency::unreserve(&who, excess);
Copy link
Member

Choose a reason for hiding this comment

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

Could possibly print some defensive failure when it cannot unreserve all, but not big issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would be the case when we are not able to unreserve the required amount? This arm of if-else would be executed only when the reserved amount > deposit required. So we should always be able to unreserve the amount. Am I missing something here?

Copy link
Member

Choose a reason for hiding this comment

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

In theory yes, but in practice, sadly no. We do had a few bugs in the past and sometimes it happens that accounts are missing reserves or similar.
Just double-checking this and emitting a defensive! then gives us a hint to when this is happening such that we can investigate manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see. Will add some logging with defensive!, thanks!

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Looks very orderly, thanks!

@paritytech-review-bot paritytech-review-bot bot requested a review from a team February 18, 2025 17:27
@github-actions github-actions bot deleted a comment from UtkarshBhardwaj007 Feb 18, 2025
@github-actions github-actions bot deleted a comment from UtkarshBhardwaj007 Feb 18, 2025
@github-actions github-actions bot deleted a comment from UtkarshBhardwaj007 Feb 18, 2025
@github-actions github-actions bot deleted a comment from UtkarshBhardwaj007 Feb 18, 2025
@github-actions github-actions bot deleted a comment from UtkarshBhardwaj007 Feb 18, 2025
@github-actions github-actions bot deleted a comment from UtkarshBhardwaj007 Feb 18, 2025
Copy link
Contributor

Command "--clean" has started 🚀 See logs here

Copy link
Contributor

Command "--clean" has finished ✅ See logs here

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework. T2-pallets This PR/Issue is related to a particular pallet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants