Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

[Feature] Add deposit to fast-unstake #12366

Merged
merged 22 commits into from
Sep 27, 2022

Conversation

ruseinov
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Sep 27, 2022
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.

Logic looks good, but please:

  1. use a smaller number for deposit in mock.rs
  2. don't hardcode the value anywhere (unless if really needed)

ruseinov and others added 2 commits September 27, 2022 16:34
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
@ruseinov ruseinov added C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. B7-runtimenoteworthy labels Sep 27, 2022
if !remaining.is_zero() {
frame_support::defensive!("`not enough balance to unreserve`");
ErasToCheckPerBlock::<T>::put(0);
Self::deposit_event(Event::<T>::InternalError)
Copy link
Contributor

Choose a reason for hiding this comment

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

This then goes on to be Ok(()) - should it be an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's alright, it has defensive checks, what this basically means is that this should always be a defensive path and if it happens - that'd panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And in prod scenario this simply disables the pallet until further notice.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes -- a defensive code path is one that should never happen. We're just coding what should happen in case there's a bug. Returning Ok(()), and

ErasToCheckPerBlock::<T>::put(0);
Self::deposit_event(Event::<T>::InternalError)

is as good as it gets.

@ruseinov
Copy link
Contributor Author

/cmd queue -c fmt $ 1

@command-bot
Copy link

command-bot bot commented Sep 27, 2022

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

Comment /cmd cancel 79-d9fc1784-c98f-4de4-a089-d17497f50a11 to cancel this command or /cmd cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Sep 27, 2022

@ruseinov Command "$PIPELINE_SCRIPTS_DIR/fmt.sh" 1 has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1890181 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1890181/artifacts/download.

@ruseinov
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot paritytech-processbot bot merged commit 94b9646 into master Sep 27, 2022
@paritytech-processbot paritytech-processbot bot deleted the ru/feature/unstake-deposit branch September 27, 2022 17:31
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* [Feature] Add deposit to fast-unstake

* disable on ErasToCheckPerBlock == 0

* removed signed ext

* remove obsolete import

* remove some obsolete stuff

* fix some comments

* fixed all the comments

* remove obsolete imports

* fix some tests

* CallNotAllowed tests

* Update frame/fast-unstake/src/lib.rs

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* fix tests

* fix deregister + tests

* more fixes

* make sure we go above existential deposit

* fixed the last test

* some nit fixes

* fix node

* fix bench

* last bench fix

* Update frame/fast-unstake/src/lib.rs

* ".git/.scripts/fmt.sh" 1

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: command-bot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants