-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Emit event when changing total locked value in pallet-balances #12287
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 (code-wise; don't know enough to reason about business logic)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting it up for audit 👍
bot rebase |
Rebased |
frame/balances/src/lib.rs
Outdated
|
||
let deposit_lock_event = |prev: T::Balance, after: T::Balance, reason: Reasons| { | ||
if prev < after { | ||
let amount = after.saturating_sub(prev); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it okay to use non-saturating math here or is using saturating math only enforced meanwhile?
Would be okay here. But since you already have it - its doing no harm.
I think both is fine. In both cases you either dont have the diff or not the total but putting both in the event could be done 🤷♂️ |
bot rebase |
Error: Command 'Command { std: "git" "push" "--porcelain" "sea212" "balances-add-lock-events", kill_on_drop: false }' failed with status Some(1); output: error: failed to push some refs to 'https://x-access-token:${SECRET}@github.com/sea212/substrate.git' |
@sea212 please merge master. |
d4f3684
to
300466d
Compare
Does anyone know what is going on here in regards to the CI? If I run the command locally all tests pass. The changes in this PR are even not related to the errors that occur. |
Error: Statuses failed for d01e57b |
bot rebase |
Rebased |
bot merge |
Waiting for commit status. |
Merge cancelled due to error. Error: Statuses failed for 2e8c08d |
The CI pipeline was cancelled due to failure one of the required jobs. |
Master is red now, but there were internal CI problems. Will fix. |
…ytech#12287) * Emit Locked/Unlocked events * Implement lock event tests * Adhere to style guide * Use saturating math Co-authored-by: Kian Paimani <[email protected]> * Fix typo * Emit event on change locks and add tests * Adjust event docstring --------- Co-authored-by: Kian Paimani <[email protected]> Co-authored-by: parity-processbot <>
Fixes #12276
This PR adds two events to the balances pallet, namely:
Since locks in the same category of
Reasons
overlay each other, the design decision was made to include only the changes of total lock value in theamount
fields of the respective events. Examples:There are also alternative approaches that could be applied:
total_locked
field to the respective events.total_locked
instead of theamount
that was added to or removed from the total lock value.Some questions in regards to the actual changes in this PR:
Reasons::All
is not used in the event. There is a corner case when a lock is set withReasons::All
that implies the same relative change of lock amounts for bothReasons::Misc
andReasons::Fee
(all currently available reasons). In that case the event could emitReasons::All
. I decided against adding this because in terms of implementation effort this does not scale well at all. Instead, two distinct events are now emitted.if a > b then a - b
andif a < b then b - a
. Is it okay to use non-saturating math here or is using saturating math only enforced meanwhile?