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.

Sign up for GitHub

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

Jump to bottom

Generic slashing side-effects #5623

Merged
merged 32 commits into from
Oct 7, 2024
Merged

Generic slashing side-effects #5623

merged 32 commits into from
Oct 7, 2024

Conversation

JuaniRios
Copy link
Contributor

@JuaniRios JuaniRios commented Sep 6, 2024 •
edited
Loading

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?

Polkadot address: 15fj1UhQp8Xes7y7LSmDYTy349mXvUwrbNmLaP5tQKBxsQY1

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
@lrazovic lrazovic mentioned this pull request Sep 10, 2024
Merged
JuaniRios added a commit to Polimec/polimec-node that referenced this pull request Sep 12, 2024
@JuaniRios
✨ Slashing behavior for pallet vesting (#396)
b0681c3 
## 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.
franciscoaguirre
franciscoaguirre approved these changes Sep 13, 2024
View reviewed changes
prdoc/pr_5623.prdoc Outdated Show resolved Hide resolved
prdoc/pr_5623.prdoc Outdated Show resolved Hide resolved
kianenigma
kianenigma reviewed Sep 13, 2024
View reviewed changes
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

@JuaniRios
Copy link
Contributor Author

@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?

@franciscoaguirre
Copy link
Contributor

bot fmt

@command-bot
Copy link

command-bot bot commented Sep 30, 2024 •
edited
Loading

@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
@JuaniRios
Merge branch 'master' into slashing-trait
bf81c85 
# Conflicts:
#	substrate/utils/wasm-builder/src/wasm_project.rs
pgherveou
pgherveou approved these changes Oct 1, 2024
View reviewed changes
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pgherveou pgherveou pgherveou approved these changes

@athei athei athei approved these changes

@franciscoaguirre franciscoaguirre franciscoaguirre approved these changes

@kianenigma kianenigma Awaiting requested review from kianenigma

@koute koute Awaiting requested review from koute

Assignees

@JuaniRios JuaniRios

Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@JuaniRios @kianenigma @franciscoaguirre @pgherveou @athei