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

Generic slashing side-effects #5623

Merged
merged 32 commits into from
Oct 7, 2024

Conversation

JuaniRios
Copy link
Contributor

@JuaniRios JuaniRios commented Sep 6, 2024

Polkadot address: 15fj1UhQp8Xes7y7LSmDYTy349mXvUwrbNmLaP5tQKBxsQY1

Description

What?

Make it possible for other pallets to implement their own logic when a slash on a balance occurs.

Why?

In the introduction of holds @gavofyork said:

Since Holds are designed to be infallibly slashed, this means that any logic using a Freeze must handle the possibility of the frozen amount being reduced, potentially to zero. A permissionless function should be provided in order to allow bookkeeping to be updated in this instance.

At Polimec we needed to find a way to reduce the vesting schedules of our users after a slash was made, and after talking to @kianenigma at the Web3Summit, we realized there was no easy way to implement this with the current traits, so we came up with this solution.

How?

  • First we abstract the done_slash function of holds::Balanced to it's own trait that any pallet can implement.
  • Then we add a config type in pallet-balances that accepts a callback tuple of all the pallets that implement this trait.
  • Finally implement done_slash for pallet-balances such that it calls the config type.

Integration

The default implementation of done_slash is still an empty function, and the new config type of pallet-balances can be set to an empty tuple, so nothing changes by default.

Review Notes

  • I suggest to focus on the first commit which contains the main logic changes.
  • I also have a working implementation of done_slash for pallet_vesting, should I add it to this PR?
  • If I run cargo +nightly fmt --all then I get changes to a lot of unrelated crates, so not sure if I should run it to avoid the fmt failure of the CI
  • Should I hunt down references to fungible/fungibles documentation and update it accordingly?

Checklist

  • My PR includes a detailed description as outlined in the "Description" and its two subsections above.
  • My PR follows the labeling requirements of this project (at minimum one label for T required)
    • External contributors: ask maintainers to put the right label on your PR.
  • I have made corresponding changes to the documentation (if applicable)

@JuaniRios JuaniRios changed the title modify frame_support trait and pallet-balances impl Generic slashing side-effects Sep 6, 2024
@JuaniRios JuaniRios marked this pull request as ready for review September 6, 2024 14:14
@JuaniRios JuaniRios requested a review from a team as a code owner September 6, 2024 14:14
@franciscoaguirre franciscoaguirre added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Sep 6, 2024
JuaniRios added a commit to Polimec/polimec-node that referenced this pull request Sep 12, 2024
## What?
- Reduce vesting schedules of pallet-vesting after a slash is made when an evaluation is settled

## Why?
- A user could have negative transferable balance if they had some tokens locked for vesting and then got slashed.

## How?
- Semi-generic solution which should be easily adapted to the Polkadot SDK. PR is [here](paritytech/polkadot-sdk#5623).
- pallet-funding (in the future pallet-balances) accepts a tuple of items that implement a trait called on_slash.
- pallet funding calls this after slashing the evaluator (we don't use the slash interface so we call the trait directly. In the future this trait should also be called when using the slash function)
- We implement on pallet vesting the trait where we see how many tokens should be released at the moment of slashing, and then apply the slash on the remaining frozen amount. We recalculate the per_block amount to keep the same end block 

## Testing?
- 2 tests in the new crate on-slash-vesting
- 1 integration test

## Anything Else?
- For now the trait for slashing needs to be in the same crate we implement it since we can't impl a foreign trait on a foreign crate.
- Soon we should submit a PR to polkadot-sdk where we submit this new slash interface on the tokens::fungible trait, and also add our vesting impl directly inside pallet-vesting.
prdoc/pr_5623.prdoc Outdated Show resolved Hide resolved
prdoc/pr_5623.prdoc Outdated Show resolved Hide resolved
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.

LGTM modulo some docs missing

@command-bot
Copy link

command-bot bot commented Sep 30, 2024

@franciscoaguirre https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7467206 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 1-67ac7272-f315-4990-a5ea-91bf27062f58 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Sep 30, 2024

@franciscoaguirre Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7467206 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7467206/artifacts/download.

@paritytech-review-bot paritytech-review-bot bot requested a review from a team September 30, 2024 15:22
# Conflicts:
#	substrate/utils/wasm-builder/src/wasm_project.rs
Copy link
Contributor

@pgherveou pgherveou left a comment

Choose a reason for hiding this comment

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

for owner check, on contracts-pallet

@kianenigma
Copy link
Contributor

@franciscoaguirre @kianenigma PR is passing the main checks. We only need a call to the fmt bot, and I think that will also fix the umbrella check.

I ran python3 ./scripts/generate-umbrella.py --sdk . --version 0.1.0 for the umbrella fix, is that the right command?

I believe so, but I will let @ggwpez chime in.

Let's use this as an opportunity to make sure our CONTRIBUTING.md has all the right steps wrt touching the umbrella crates.

@franciscoaguirre franciscoaguirre added this pull request to the merge queue Oct 2, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 2, 2024
@franciscoaguirre franciscoaguirre added this pull request to the merge queue Oct 2, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 2, 2024
@franciscoaguirre franciscoaguirre added this pull request to the merge queue Oct 4, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 4, 2024
@franciscoaguirre franciscoaguirre added this pull request to the merge queue Oct 7, 2024
Merged via the queue into paritytech:master with commit c0ddfba Oct 7, 2024
217 checks passed
@kianenigma
Copy link
Contributor

\tip medium

@kianenigma
Copy link
Contributor

/tip medium

Copy link

@JuaniRios Contributor did not properly post their account address.

Make sure the pull request description (or user bio) has: "{network} address: {address}".

@kianenigma
Copy link
Contributor

/tip medium

2 similar comments
@kianenigma
Copy link
Contributor

/tip medium

@kianenigma
Copy link
Contributor

/tip medium

Copy link

@kianenigma A referendum for a medium (80 DOT) tip was successfully submitted for @JuaniRios (15fj1UhQp8Xes7y7LSmDYTy349mXvUwrbNmLaP5tQKBxsQY1 on polkadot).

Referendum number: 1236.
tip

Copy link

The referendum has appeared on Polkassembly.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants