docs(x/accounts/defaults/lockup): Add slash document for lockup account #22783
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces a new section titled "In An Event Of Slashing" to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
x/accounts/defaults/lockup/README.md(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
x/accounts/defaults/lockup/README.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🪛 LanguageTool
x/accounts/defaults/lockup/README.md
[grammar] ~114-~114: Consider using either the past participle “slashed” or the present participle “slashing” here.
Context: ... that the lockup account delegate to is slash which affect the actual delegation amou...
(BEEN_PART_AGREEMENT)
[style] ~114-~114: Consider removing “of” to be more concise
Context: ...n excess amount even if user undelegate all of the account delegated amount. This excess ...
(ALL_OF_THE)
[uncategorized] ~134-~134: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...annot assume certain account types exist so the handling of slashing event would ha...
(COMMA_COMPOUND_SENTENCE_2)
[grammar] ~134-~134: This is normally spelled as one word.
Context: ... of lockup account would make the logic over complicated. As the above effects are only an edge ...
(OVER_COMPOUNDS)
[uncategorized] ~134-~134: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...e only an edge case that affect a small amount of users, so here we would accept the t...
(AMOUNTOF_TO_NUMBEROF)
[uncategorized] ~134-~134: When ‘trade-off’ is used as a noun or modifier, it needs to be hyphenated.
Context: ...t of users, so here we would accept the trade off for a simpler design. The same design i...
(VERB_NOUN_CONFUSION)
🔇 Additional comments (2)
x/accounts/defaults/lockup/README.md (2)
11-11: LGTM!
The table of contents update is correctly formatted and properly linked.
234-234: LGTM!
The reference to the slashing section is properly added and helps readers understand the excess DV amount scenario.
x/accounts/defaults/lockup/README.md
Outdated
| ## In An Event Of Slashing | ||
|
|
||
| As defined, base lockup store `DelegatedLocking` by amount. In an event of a validator that the lockup account delegate to is slash which affect the actual delegation amount, this will leave the `DelegatedLocking` have an excess amount even if user undelegate all of the | ||
| account delegated amount. This excess amount would affect the spendable amount, further details are as below: | ||
|
|
||
| Spendable amount are calculated as | ||
| `spendableAmount` = `balance` - `notBondedLockedAmout` | ||
| Whereas `notBondedLockedAmout` = `lockedAmount` - `Min(LockedAmount, DelegatedLockedAmount)` | ||
|
|
||
| As seen in the formula `notBondedLockedAmout` can only be 0 or a positive value when `DelegatedLockedAmount` < `LockedAmount` | ||
| if the `notBondedLockedAmout` is positive then `spendableAmount` is less than the actual balance. Let call `NewDelegatedLockedAmount` is the `delegatedLockedAmount` when applying N slash | ||
|
|
||
| 1. Case 1: where originally `DelegatedLockedAmount` > `lockedAmount` but when applying the slash amount the `NewDelegatedLockedAmount` < `lockedAmount` then | ||
| * When not applying slash `notBondedLockedAmout` will be 0 | ||
| * When apply slash `notBondedLockedAmout` will be `lockedAmount` - `NewDelegatedLockedAmount` = a positive amount | ||
| 2. Case 2: where originally `DelegatedLockedAmount` < `lockedAmount` when applying the slash amount the `NewDelegatedLockedAmount` < `lockedAmount` then | ||
| * When not applying slash `lockedAmount` - `DelegatedLockedAmount` | ||
| * When apply slash `notBondedLockedAmout` will be `lockedAmount` - `NewDelegatedLockedAmount` = `lockedAmount` - `(DelegatedLockedAmount - N)` = `lockedAmount` - `DelegatedLockedAmount` + N | ||
| 3. Case 3: where originally `DelegatedLockedAmount` > `lockedAmount` when applying the slash amount still the `NewDelegatedLockedAmount` > `lockedAmount` then `notBondedLockedAmout` will be 0 applying slash or not | ||
|
|
||
| In the 3 cases, case 1 and case 2 seen the `notBondedLockedAmout` decrease when not applying the slash, make the `spendableAmount` higher. | ||
|
|
||
| Due to the natural of x/accounts, as other modules cannot assume certain account types exist so the handling of slashing event would have to be done internally inside x/accounts's accounts which in the case of lockup account would make the logic over complicated. As the above effects are only an edge case that affect a small amount of users, so here we would accept the trade off for a simpler design. The same design intention is also present in the legacy vesting account. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve clarity and fix grammatical issues in the slashing documentation
The technical content is accurate, but the section needs improvements in clarity and grammar:
Apply these changes:
-As defined, base lockup store `DelegatedLocking` by amount. In an event of a validator that the lockup account delegate to is slash which affect the actual delegation amount, this will leave the `DelegatedLocking` have an excess amount even if user undelegate all of the
-account delegated amount. This excess amount would affect the spendable amount, further details are as below:
+The base lockup stores `DelegatedLocking` by amount. When a validator that the lockup account is delegated to gets slashed, affecting the actual delegation amount, the `DelegatedLocking` will have an excess amount even if the user undelegates all delegated amounts. This excess amount affects the spendable amount as detailed below:
-Spendable amount are calculated as
-`spendableAmount` = `balance` - `notBondedLockedAmout`
-Whereas `notBondedLockedAmout` = `lockedAmount` - `Min(LockedAmount, DelegatedLockedAmount)`
+The spendable amount is calculated as:
+```
+spendableAmount = balance - notBondedLockedAmount
+where notBondedLockedAmount = lockedAmount - Min(lockedAmount, delegatedLockedAmount)
+```
-As seen in the formula `notBondedLockedAmout` can only be 0 or a positive value when `DelegatedLockedAmount` < `LockedAmount`
-if the `notBondedLockedAmout` is positive then `spendableAmount` is less than the actual balance. Let call `NewDelegatedLockedAmount` is the `delegatedLockedAmount` when applying N slash
+As seen in the formula, `notBondedLockedAmount` can only be 0 or a positive value when `delegatedLockedAmount` < `lockedAmount`.
+If `notBondedLockedAmount` is positive, then `spendableAmount` is less than the actual balance. Let's define `newDelegatedLockedAmount` as the `delegatedLockedAmount` after applying N slashes.
-1. Case 1: where originally `DelegatedLockedAmount` > `lockedAmount` but when applying the slash amount the `NewDelegatedLockedAmount` < `lockedAmount` then
- * When not applying slash `notBondedLockedAmout` will be 0
- * When apply slash `notBondedLockedAmout` will be `lockedAmount` - `NewDelegatedLockedAmount` = a positive amount
+1. Case 1: Originally `delegatedLockedAmount` > `lockedAmount`, but after slashing, `newDelegatedLockedAmount` < `lockedAmount`:
+ * Before slashing: `notBondedLockedAmount = 0`
+ * After slashing: `notBondedLockedAmount = lockedAmount - newDelegatedLockedAmount` (a positive amount)
-2. Case 2: where originally `DelegatedLockedAmount` < `lockedAmount` when applying the slash amount the `NewDelegatedLockedAmount` < `lockedAmount` then
- * When not applying slash `lockedAmount` - `DelegatedLockedAmount`
- * When apply slash `notBondedLockedAmout` will be `lockedAmount` - `NewDelegatedLockedAmount` = `lockedAmount` - `(DelegatedLockedAmount - N)` = `lockedAmount` - `DelegatedLockedAmount` + N
+2. Case 2: Originally `delegatedLockedAmount` < `lockedAmount`, and after slashing, `newDelegatedLockedAmount` < `lockedAmount`:
+ * Before slashing: `notBondedLockedAmount = lockedAmount - delegatedLockedAmount`
+ * After slashing: `notBondedLockedAmount = lockedAmount - newDelegatedLockedAmount = lockedAmount - (delegatedLockedAmount - N) = lockedAmount - delegatedLockedAmount + N`
-3. Case 3: where originally `DelegatedLockedAmount` > `lockedAmount` when applying the slash amount still the `NewDelegatedLockedAmount` > `lockedAmount` then `notBondedLockedAmout` will be 0 applying slash or not
+3. Case 3: Originally `delegatedLockedAmount` > `lockedAmount`, and after slashing, `newDelegatedLockedAmount` > `lockedAmount`:
+ * Both before and after slashing: `notBondedLockedAmount = 0`
-In the 3 cases, case 1 and case 2 seen the `notBondedLockedAmout` decrease when not applying the slash, make the `spendableAmount` higher.
+In cases 1 and 2, `notBondedLockedAmount` decreases when not applying the slash, resulting in a higher `spendableAmount`.
-Due to the natural of x/accounts, as other modules cannot assume certain account types exist so the handling of slashing event would have to be done internally inside x/accounts's accounts which in the case of lockup account would make the logic over complicated. As the above effects are only an edge case that affect a small amount of users, so here we would accept the trade off for a simpler design. The same design intention is also present in the legacy vesting account.
+Due to the nature of x/accounts, where other modules cannot assume certain account types exist, the handling of slashing events must be done internally within x/accounts. For lockup accounts, this would make the logic overcomplicated. Since these effects only impact a small number of users, we accept the trade-off for a simpler design. This design decision aligns with the legacy vesting account implementation.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## In An Event Of Slashing | |
| As defined, base lockup store `DelegatedLocking` by amount. In an event of a validator that the lockup account delegate to is slash which affect the actual delegation amount, this will leave the `DelegatedLocking` have an excess amount even if user undelegate all of the | |
| account delegated amount. This excess amount would affect the spendable amount, further details are as below: | |
| Spendable amount are calculated as | |
| `spendableAmount` = `balance` - `notBondedLockedAmout` | |
| Whereas `notBondedLockedAmout` = `lockedAmount` - `Min(LockedAmount, DelegatedLockedAmount)` | |
| As seen in the formula `notBondedLockedAmout` can only be 0 or a positive value when `DelegatedLockedAmount` < `LockedAmount` | |
| if the `notBondedLockedAmout` is positive then `spendableAmount` is less than the actual balance. Let call `NewDelegatedLockedAmount` is the `delegatedLockedAmount` when applying N slash | |
| 1. Case 1: where originally `DelegatedLockedAmount` > `lockedAmount` but when applying the slash amount the `NewDelegatedLockedAmount` < `lockedAmount` then | |
| * When not applying slash `notBondedLockedAmout` will be 0 | |
| * When apply slash `notBondedLockedAmout` will be `lockedAmount` - `NewDelegatedLockedAmount` = a positive amount | |
| 2. Case 2: where originally `DelegatedLockedAmount` < `lockedAmount` when applying the slash amount the `NewDelegatedLockedAmount` < `lockedAmount` then | |
| * When not applying slash `lockedAmount` - `DelegatedLockedAmount` | |
| * When apply slash `notBondedLockedAmout` will be `lockedAmount` - `NewDelegatedLockedAmount` = `lockedAmount` - `(DelegatedLockedAmount - N)` = `lockedAmount` - `DelegatedLockedAmount` + N | |
| 3. Case 3: where originally `DelegatedLockedAmount` > `lockedAmount` when applying the slash amount still the `NewDelegatedLockedAmount` > `lockedAmount` then `notBondedLockedAmout` will be 0 applying slash or not | |
| In the 3 cases, case 1 and case 2 seen the `notBondedLockedAmout` decrease when not applying the slash, make the `spendableAmount` higher. | |
| Due to the natural of x/accounts, as other modules cannot assume certain account types exist so the handling of slashing event would have to be done internally inside x/accounts's accounts which in the case of lockup account would make the logic over complicated. As the above effects are only an edge case that affect a small amount of users, so here we would accept the trade off for a simpler design. The same design intention is also present in the legacy vesting account. | |
| ## In An Event Of Slashing | |
| The base lockup stores `DelegatedLocking` by amount. When a validator that the lockup account is delegated to gets slashed, affecting the actual delegation amount, the `DelegatedLocking` will have an excess amount even if the user undelegates all delegated amounts. This excess amount affects the spendable amount as detailed below: | |
| The spendable amount is calculated as: |
🧰 Tools
🪛 LanguageTool
[grammar] ~114-~114: Consider using either the past participle “slashed” or the present participle “slashing” here.
Context: ... that the lockup account delegate to is slash which affect the actual delegation amou...
(BEEN_PART_AGREEMENT)
[style] ~114-~114: Consider removing “of” to be more concise
Context: ...n excess amount even if user undelegate all of the account delegated amount. This excess ...
(ALL_OF_THE)
[uncategorized] ~134-~134: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...annot assume certain account types exist so the handling of slashing event would ha...
(COMMA_COMPOUND_SENTENCE_2)
[grammar] ~134-~134: This is normally spelled as one word.
Context: ... of lockup account would make the logic over complicated. As the above effects are only an edge ...
(OVER_COMPOUNDS)
[uncategorized] ~134-~134: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...e only an edge case that affect a small amount of users, so here we would accept the t...
(AMOUNTOF_TO_NUMBEROF)
[uncategorized] ~134-~134: When ‘trade-off’ is used as a noun or modifier, it needs to be hyphenated.
Context: ...t of users, so here we would accept the trade off for a simpler design. The same design i...
(VERB_NOUN_CONFUSION)
julienrbrt
changed the title
julienrbrt
added
the
backport/v0.52.x
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
x/accounts/defaults/lockup/README.md (1)
117-119: Improve formula presentationThe formula presentation could be enhanced for better readability.
Consider using a code block with proper formatting:
-The spendable amount is calculated as: -`spendableAmount` = `balance` - `notBondedLockedAmount` -where `notBondedLockedAmount` = `lockedAmount` - `Min(lockedAmount, delegatedLockedAmount)` +The spendable amount is calculated as follows: +``` +spendableAmount = balance - notBondedLockedAmount +where: + notBondedLockedAmount = lockedAmount - Min(lockedAmount, delegatedLockedAmount) +```
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
x/accounts/defaults/lockup/README.md(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
x/accounts/defaults/lockup/README.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🪛 LanguageTool
x/accounts/defaults/lockup/README.md
[uncategorized] ~114-~114: This verb may not be in the correct form. Consider using a different form for this context.
Context: ... that the lockup account delegate to is slash which affect the actual delegation amou...
(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
[uncategorized] ~114-~114: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...ckup account delegate to is slash which affect the actual delegation amount, this will...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~114-~114: You might be missing the article “the” here.
Context: ...dLocking` have an excess amount even if user undelegate all of the account delegate...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[style] ~114-~114: Consider removing “of” to be more concise
Context: ...n excess amount even if user undelegate all of the account delegated amount. This excess ...
(ALL_OF_THE)
[uncategorized] ~133-~133: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...annot assume certain account types exist so the handling of slashing event must be ...
(COMMA_COMPOUND_SENTENCE_2)
[style] ~133-~133: Specify a number, remove phrase, use “a few”, or use “some”
Context: ...fects are only an edge case that affect a small number of users, so here we would accept the trad...
(SMALL_NUMBER_OF)
[uncategorized] ~133-~133: When ‘trade-off’ is used as a noun or modifier, it needs to be hyphenated.
Context: ...r of users, so here we would accept the trade off for a simpler design. This design decis...
(VERB_NOUN_CONFUSION)
🔇 Additional comments (3)
x/accounts/defaults/lockup/README.md (3)
11-11: LGTM: Table of contents entry is correctly formatted
The new section is properly added to the table of contents with the correct link format.
233-233: LGTM: Helpful cross-reference added
The reference to the slashing section is well-placed and helps readers understand the excess DV amount scenario.
114-115: 🛠️ Refactor suggestion
Fix grammar and clarity in the introduction
The opening paragraph needs grammatical improvements for better clarity.
Apply this change:
-As defined, base lockup store `DelegatedLocking` by amount. In an event of a validator that the lockup account delegate to is slash which affect the actual delegation amount, this will leave the `DelegatedLocking` have an excess amount even if user undelegate all of the
-account delegated amount. This excess amount would affect the spendable amount, further details are as below:
+The base lockup stores delegation amounts in `DelegatedLocking`. When a validator that the lockup account is delegated to gets slashed, affecting the actual delegation amount, the `DelegatedLocking` may retain an excess amount even after the user undelegates all delegated amounts. This excess amount affects the spendable amount as detailed below:Likely invalid or redundant comment.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~114-~114: This verb may not be in the correct form. Consider using a different form for this context.
Context: ... that the lockup account delegate to is slash which affect the actual delegation amou...
(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
[uncategorized] ~114-~114: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...ckup account delegate to is slash which affect the actual delegation amount, this will...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~114-~114: You might be missing the article “the” here.
Context: ...dLocking` have an excess amount even if user undelegate all of the account delegate...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[style] ~114-~114: Consider removing “of” to be more concise
Context: ...n excess amount even if user undelegate all of the account delegated amount. This excess ...
(ALL_OF_THE)
| As seen in the formula `notBondedLockedAmout` can only be 0 or a positive value when `DelegatedLockedAmount` < `LockedAmount`. Let call `NewDelegatedLockedAmount` is the `delegatedLockedAmount` when applying N slash | ||
|
|
||
| 1. Case 1: Originally `DelegatedLockedAmount` > `lockedAmount` but when applying the slash amount the `NewDelegatedLockedAmount` < `lockedAmount` then | ||
| * When not applying slash `notBondedLockedAmout` will be 0 | ||
| * When apply slash `notBondedLockedAmout` will be `lockedAmount` - `NewDelegatedLockedAmount` = a positive amount | ||
| 2. Case 2: where originally `DelegatedLockedAmount` < `lockedAmount` when applying the slash amount the `NewDelegatedLockedAmount` < `lockedAmount` then | ||
| * When not applying slash `lockedAmount` - `DelegatedLockedAmount` | ||
| * When apply slash `notBondedLockedAmout` will be `lockedAmount` - `NewDelegatedLockedAmount` = `lockedAmount` - `(DelegatedLockedAmount - N)` = `lockedAmount` - `DelegatedLockedAmount` + N | ||
| 3. Case 3: where originally `DelegatedLockedAmount` > `lockedAmount` when applying the slash amount still the `NewDelegatedLockedAmount` > `lockedAmount` then `notBondedLockedAmout` will be 0 applying slash or not |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix inconsistencies and improve case descriptions
There are inconsistencies in terminology and formatting of the cases.
Apply these changes:
- Fix spelling:
notBondedLockedAmout→notBondedLockedAmount - Standardize variable names:
DelegatedLockedAmount→delegatedLockedAmount - Improve case formatting for consistency
- Add proper code blocks for formulas
Would you like me to provide the complete diff for these changes?
|
|
||
| In cases 1 and 2, `notBondedLockedAmount` decreases when not applying the slash, resulting in a higher `spendableAmount`. | ||
|
|
||
| Due to the nature of x/accounts, as other modules cannot assume certain account types exist so the handling of slashing event must be done internally within x/accounts's accounts. For lockup accounts, this would make the logic overcomplicated. Since these effects are only an edge case that affect a small number of users, so here we would accept the trade off for a simpler design. This design decision aligns with the legacy vesting account implementation. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix grammar and hyphenation in the conclusion
The concluding paragraph needs grammatical improvements and proper hyphenation.
Apply this change:
-Due to the nature of x/accounts, as other modules cannot assume certain account types exist so the handling of slashing event must be done internally within x/accounts's accounts. For lockup accounts, this would make the logic overcomplicated. Since these effects are only an edge case that affect a small number of users, so here we would accept the trade off for a simpler design. This design decision aligns with the legacy vesting account implementation.
+Due to the nature of x/accounts, where other modules cannot assume certain account types exist, the handling of slashing events must be done internally within x/accounts. For lockup accounts, this would make the logic overcomplicated. Since these effects only impact a small number of users, we accept the trade-off for a simpler design. This design decision aligns with the legacy vesting account implementation.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Due to the nature of x/accounts, as other modules cannot assume certain account types exist so the handling of slashing event must be done internally within x/accounts's accounts. For lockup accounts, this would make the logic overcomplicated. Since these effects are only an edge case that affect a small number of users, so here we would accept the trade off for a simpler design. This design decision aligns with the legacy vesting account implementation. | |
| Due to the nature of x/accounts, where other modules cannot assume certain account types exist, the handling of slashing events must be done internally within x/accounts. For lockup accounts, this would make the logic overcomplicated. Since these effects only impact a small number of users, we accept the trade-off for a simpler design. This design decision aligns with the legacy vesting account implementation. |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~133-~133: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...annot assume certain account types exist so the handling of slashing event must be ...
(COMMA_COMPOUND_SENTENCE_2)
[style] ~133-~133: Specify a number, remove phrase, use “a few”, or use “some”
Context: ...fects are only an edge case that affect a small number of users, so here we would accept the trad...
(SMALL_NUMBER_OF)
[uncategorized] ~133-~133: When ‘trade-off’ is used as a noun or modifier, it needs to be hyphenated.
Context: ...r of users, so here we would accept the trade off for a simpler design. This design decis...
(VERB_NOUN_CONFUSION)
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Documentation