[Supplier] Supplier unbonding period governance param #704
Conversation
red-0ne
added
supplier
WalkthroughThe changes introduce two new fields, Changes
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![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
Outside diff range, codebase verification and nitpick comments (17)
docusaurus/docs/protocol/actors/supplier.md (12)
80-80: Add a period at the end of the sentence.The sentence should end with a period for consistency.
- ## Supplier lifecycle + ## Supplier lifecycle.
85-85: Remove trailing punctuation in heading.Remove the colon at the end of the heading.
- ### 1. Staking Initiation: + ### 1. Staking InitiationTools
Markdownlint
85-85: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
87-89: Fix unordered list style and indentation.Use dashes for unordered lists and correct the indentation.
- * The `Supplier` sends a `MsgStakeSupplier` message to the network, initiating - the staking process. - * The `Supplier` remains inactive until the beginning of the next session. + - The `Supplier` sends a `MsgStakeSupplier` message to the network, initiating + the staking process. + - The `Supplier` remains inactive until the beginning of the next session.Tools
Markdownlint
87-87: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
89-89: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
87-87: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
89-89: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
91-91: Remove trailing punctuation in heading.Remove the colon at the end of the heading.
- ### 2. Activation: + ### 2. ActivationTools
Markdownlint
91-91: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
93-94: Fix unordered list style and indentation.Use dashes for unordered lists and correct the indentation.
- * The `Supplier` becomes active at the start of the new session and can now - provide services to the network. + - The `Supplier` becomes active at the start of the new session and can now + provide services to the network.Tools
Markdownlint
93-93: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
93-93: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
98-100: Fix unordered list style and indentation.Use dashes for unordered lists and correct the indentation.
- * The `Supplier` sends a `MsgUnstakeSupplier` message to the network to start - the unstaking process. - * The `Supplier` continues to be active until the end of the current session. + - The `Supplier` sends a `MsgUnstakeSupplier` message to the network to start + the unstaking process. + - The `Supplier` continues to be active until the end of the current session.Tools
Markdownlint
98-98: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
100-100: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
98-98: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
100-100: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
102-102: Remove trailing punctuation in heading.Remove the colon at the end of the heading.
- ### 4. Unbonding Phase: + ### 4. Unbonding PhaseTools
Markdownlint
102-102: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
104-106: Fix unordered list style and indentation.Use dashes for unordered lists and correct the indentation.
- * In the following session, the `Supplier` enters the unbonding phase, becoming - inactive and no longer participating in sessions or providing services. - * During this phase, the staked funds are locked, and any pending claims are settled. + - In the following session, the `Supplier` enters the unbonding phase, becoming + inactive and no longer participating in sessions or providing services. + - During this phase, the staked funds are locked, and any pending claims are settled.Tools
Markdownlint
104-104: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
106-106: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
104-104: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
106-106: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
108-108: Remove trailing punctuation in heading.Remove the colon at the end of the heading.
- ### 5. Completion of Unbonding: + ### 5. Completion of UnbondingTools
Markdownlint
108-108: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
110-111: Fix unordered list style and indentation.Use dashes for unordered lists and correct the indentation.
- * After the unbonding period ends, the staked funds are transferred back to the - `Supplier`'s account, and the `Supplier`'s record is removed from the network. + - After the unbonding period ends, the staked funds are transferred back to the + `Supplier`'s account, and the `Supplier`'s record is removed from the network.Tools
Markdownlint
110-110: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
110-110: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
113-113: Remove trailing punctuation in heading.Remove the colon at the end of the heading.
- ### Timeline of Events: + ### Timeline of Events
118-120: Fix unordered list style and indentation.Use dashes for unordered lists and correct the indentation.
- * **Unbonding Period**: 3 sessions - * **Number of Blocks per Session**: 10 blocks - * **Session End Height to Proof Window Close Height**: 15 blocks + - **Unbonding Period**: 3 sessions + - **Number of Blocks per Session**: 10 blocks + - **Session End Height to Proof Window Close Height**: 15 blocksTools
Markdownlint
118-118: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
119-119: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
120-120: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
x/shared/types/params.go (1)
20-21: Add a comment forParamSupplierUnbondingPeriodSessions.The comment for
ParamSupplierUnbondingPeriodSessionsis missing. Add a comment to maintain consistency.+ // ParamSupplierUnbondingPeriodSessions is the key for the supplier unbonding period sessions parameter. ParamSupplierUnbondingPeriodSessions = "supplier_unbonding_period_sessions"x/shared/keeper/msg_server_update_param_test.go (4)
52-58: Consider renamingparamDeltatodeltaClaimWindowOpenOffsetBlocks.The variable name
paramDeltacould be more descriptive to indicate its specific purpose.- paramDelta := uint64(expectedClaimWindowOpenOffestBlocks) - defaultParams.ClaimWindowOpenOffsetBlocks + deltaClaimWindowOpenOffsetBlocks := uint64(expectedClaimWindowOpenOffestBlocks) - defaultParams.ClaimWindowOpenOffsetBlocks - defaultParams.SupplierUnbondingPeriodSessions = getMinSupplierUnbondingPeriodSessions(&defaultParams, paramDelta) + defaultParams.SupplierUnbondingPeriodSessions = getMinSupplierUnbondingPeriodSessions(&defaultParams, deltaClaimWindowOpenOffsetBlocks)
87-93: Consider renamingparamDeltatodeltaClaimWindowCloseOffsetBlocks.The variable name
paramDeltacould be more descriptive to indicate its specific purpose.- paramDelta := uint64(expectedClaimWindowCloseOffestBlocks) - defaultParams.ClaimWindowCloseOffsetBlocks + deltaClaimWindowCloseOffsetBlocks := uint64(expectedClaimWindowCloseOffestBlocks) - defaultParams.ClaimWindowCloseOffsetBlocks - defaultParams.SupplierUnbondingPeriodSessions = getMinSupplierUnbondingPeriodSessions(&defaultParams, paramDelta) + defaultParams.SupplierUnbondingPeriodSessions = getMinSupplierUnbondingPeriodSessions(&defaultParams, deltaClaimWindowCloseOffsetBlocks)
122-128: Consider renamingparamDeltatodeltaProofWindowOpenOffsetBlocks.The variable name
paramDeltacould be more descriptive to indicate its specific purpose.- paramDelta := uint64(expectedProofWindowOpenOffestBlocks) - defaultParams.ProofWindowOpenOffsetBlocks + deltaProofWindowOpenOffsetBlocks := uint64(expectedProofWindowOpenOffestBlocks) - defaultParams.ProofWindowOpenOffsetBlocks - defaultParams.SupplierUnbondingPeriodSessions = getMinSupplierUnbondingPeriodSessions(&defaultParams, paramDelta) + defaultParams.SupplierUnbondingPeriodSessions = getMinSupplierUnbondingPeriodSessions(&defaultParams, deltaProofWindowOpenOffsetBlocks)
157-163: Consider renamingparamDeltatodeltaProofWindowCloseOffsetBlocks.The variable name
paramDeltacould be more descriptive to indicate its specific purpose.- paramDelta := uint64(expectedProofWindowCloseOffestBlocks) - defaultParams.ProofWindowCloseOffsetBlocks + deltaProofWindowCloseOffsetBlocks := uint64(expectedProofWindowCloseOffestBlocks) - defaultParams.ProofWindowCloseOffsetBlocks - defaultParams.SupplierUnbondingPeriodSessions = getMinSupplierUnbondingPeriodSessions(&defaultParams, paramDelta) + defaultParams.SupplierUnbondingPeriodSessions = getMinSupplierUnbondingPeriodSessions(&defaultParams, deltaProofWindowCloseOffsetBlocks)
| // new value to update the supplier unbonding period blocks such as it is greater | ||
| // than the cumulative proof window close blocks to pass UpdateParam validation. | ||
| paramDelta := uint64(expectedProofWindowOpenOffestBlocks) - defaultParams.ProofWindowOpenOffsetBlocks | ||
| defaultParams.SupplierUnbondingPeriodSessions = getMinSupplierUnbondingPeriodSessions(&defaultParams, paramDelta) |
There was a problem hiding this comment.
NIT: What do you think of capturing the paramDelta calculation inside of getMinSupplier....
It already takes in defaultParams so you'd just provide expectedProofWindow... instead?
There was a problem hiding this comment.
In order to achieve that, we need to know which variable is affected.
We currently do: fn([A, B, C], X-B)
Which we can't replicate by doing fn([A, B, C], X) since we lose which item we are subtracting from.
We can achieve by passing the affected original value B though. fn([A, B, C], X, B)
|
The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks. You may need to run GCP workloads (requires changing the namespace to 704) |
github-actions
bot
added
devnet
push-image
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
docusaurus/docs/protocol/actors/supplier.md (2)
18-18: Correct typo in heading.The word "Unsaking" should be "Unstaking".
- ### 3. Unsaking Initiation: + ### 3. Unstaking Initiation
87-120: Fix unordered list style and indentation.The unordered list style and indentation should be consistent.
- * The `Supplier` sends a `MsgStakeSupplier` message to the network, initiating - the staking process. - * The `Supplier` remains inactive until the beginning of the next session. + - The `Supplier` sends a `MsgStakeSupplier` message to the network, initiating + the staking process. + - The `Supplier` remains inactive until the beginning of the next session. - * The `Supplier` becomes active at the start of the new session and can now - provide services to the network. + - The `Supplier` becomes active at the start of the new session and can now + provide services to the network. - * The `Supplier` sends a `MsgUnstakeSupplier` message to the network to start - the unstaking process. - * The `Supplier` continues to be active until the end of the current session. + - The `Supplier` sends a `MsgUnstakeSupplier` message to the network to start + the unstaking process. + - The `Supplier` continues to be active until the end of the current session. - * In the following session, the `Supplier` enters the unbonding phase, becoming - inactive and no longer participating in sessions or providing services. - * During this phase, the staked funds are locked, and any pending claims are settled. + - In the following session, the `Supplier` enters the unbonding phase, becoming + inactive and no longer participating in sessions or providing services. + - During this phase, the staked funds are locked, and any pending claims are settled. - * After the unbonding period ends, the staked funds are transferred back to the - `Supplier`'s account, and the `Supplier`'s record is removed from the network. + - After the unbonding period ends, the staked funds are transferred back to the + `Supplier`'s account, and the `Supplier`'s record is removed from the network. - * **Unbonding Period**: 3 sessions - * **Number of Blocks per Session**: 10 blocks - * **Session End Height to Proof Window Close Height**: 15 blocks + - **Unbonding Period**: 3 sessions + - **Number of Blocks per Session**: 10 blocks + - **Session End Height to Proof Window Close Height**: 15 blocksTools
Markdownlint
87-87: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
89-89: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
93-93: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
98-98: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
100-100: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
104-104: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
106-106: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
110-110: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
118-118: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
119-119: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
120-120: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
87-87: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
89-89: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
93-93: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
98-98: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
100-100: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
104-104: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
106-106: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
110-110: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range, codebase verification and nitpick comments (7)
docusaurus/docs/protocol/actors/supplier.md (7)
96-96: Correct typo in heading.The word "Unsaking" should be "Unstaking".
- ### 3. Unsaking Initiation + ### 3. Unstaking Initiation
126-126: Improve readability of timeline events.Enclose status indicators in brackets for clarity.
- Block 3: DO Stake Supplier1 <br> [Is inactive] + Block 3: DO Stake Supplier1 <br> (Is inactive)
127-127: Improve readability of timeline events.Enclose status indicators in brackets for clarity.
- Block 5: DO Stake Supplier2 <br> [Is inactive] + Block 5: DO Stake Supplier2 <br> (Is inactive)
131-131: Improve readability of timeline events.Enclose status indicators in brackets for clarity.
- Block 21 : Supplier1 unstakes <br> [Still active] + Block 21 : Supplier1 unstakes <br> (Still active)
132-132: Improve readability of timeline events.Enclose status indicators in brackets for clarity.
- Block 28 : Supplier2 unstakes <br> [Still active] + Block 28 : Supplier2 unstakes <br> (Still active)
135-135: Improve readability of timeline events.Enclose status indicators in brackets for clarity.
- Block 31: Supplier1 IS NOT included in sessions : Supplier2 IS NOT included in sessions + Block 31: Supplier1 IS NOT included in sessions : Supplier2 IS NOT included in sessions
139-139: Improve readability of timeline events.Enclose status indicators in brackets for clarity.
- Block 60: DO UNBOND Supplier1 : DO UNBOND Supplier2 + Block 60: DO UNBOND Supplier1 : DO UNBOND Supplier2
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (3)
tests/integration/supplier/supplier_lifecycle.go (1)
3-13: Clarify and expand TODO comments.The TODO comments outline the steps for testing the supplier lifecycle but could benefit from additional details to ensure clarity and completeness. Consider expanding each step to include specific actions and expected outcomes.
- // 1. Stake a supplier + // 1. Stake a supplier using `MsgStakeSupplier` message. - // 2. Check that is is not included in sessions + // 2. Check that the supplier is not included in sessions immediately after staking. - // 3. Wait until it is activated + // 3. Wait until the next session starts to ensure the supplier is activated. - // 4. Check that is is included in sessions + // 4. Check that the supplier is included in sessions after activation. - // 5. Unstake the supplier mid session + // 5. Unstake the supplier mid-session using `MsgUnstakeSupplier` message. - // 6. Check that it is included in the current session + // 6. Check that the supplier remains active for the rest of the current session. - // 7. Check that it is not included in the next session + // 7. Check that the supplier is not included in the next session after unstaking. - // 8. Submit a proof and a claim and ensure it is successful + // 8. Submit a proof and a claim for the supplier and ensure it is successful. - // 9. Check that it gets rewarded when settling the claim + // 9. Check that the supplier receives rewards when settling the claim. - // 10. Check that it gets removed from the suppliers list after the unbonding period is over + // 10. Check that the supplier is removed from the suppliers list after the unbonding period is over.docusaurus/docs/protocol/actors/supplier.md (2)
18-18: Correct typo in heading.The word "Unsaking" should be "Unstaking".
- ### 3. Unsaking Initiation: + ### 3. Unstaking Initiation:
123-139: Great addition!The timeline of events provides a clear and detailed visual representation of the supplier lifecycle. Consider putting statuses like
Is inactiveandStill activein brackets for consistency.- Block 3: DO Stake Supplier1 <br> [Is inactive] - Block 5: DO Stake Supplier2 <br> [Is inactive] + Block 3: DO Stake Supplier1 <br> (Is inactive) + Block 5: DO Stake Supplier2 <br> (Is inactive) - Block 21 : Supplier1 unstakes <br> [Still active] - Block 28 : Supplier2 unstakes <br> [Still active] + Block 21 : Supplier1 unstakes <br> (Still active) + Block 28 : Supplier2 unstakes <br> (Still active)
| @@ -255,7 +263,12 @@ func TestMsgUpdateParam_UpdateSupplierUnbondingPeriodSessions(t *testing.T) { | |||
|
|
|||
| // getMinSupplierUnbondingPeriodSessions returns the minimum supplier unbonding period | |||
| // in session number that is greater than the cumulative proof window close blocks. | |||
| func getMinSupplierUnbondingPeriodSessions(params *sharedtypes.Params, deltaBlocks uint64) uint64 { | |||
| func getMinSupplierUnbondingPeriodSessions( | |||
There was a problem hiding this comment.
I like this new helper but am a bit confused why the comment everywhere it's used is 100% the same BUT the inputs we give it are different?
- Same function
- Same variable assignment
- Same comment
- Different inputs
#PUC ? What am I misunderstanding?
|
@red-0ne Reminder to update the mileston on PRs & Issues please |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (6)
x/shared/keeper/msg_server_update_param_test.go (6)
53-54: Typo in comment.The comment has a typo: "SupplierUnbondigPeriodSessions" should be "SupplierUnbondingPeriodSessions".
- // Update the SupplierUnbondigPeriodSessions such that it is greater than the + // Update the SupplierUnbondingPeriodSessions such that it is greater than the
90-91: Typo in comment.The comment has a typo: "SupplierUnbondigPeriodSessions" should be "SupplierUnbondingPeriodSessions".
- // Update the SupplierUnbondigPeriodSessions such that it is greater than the + // Update the SupplierUnbondingPeriodSessions such that it is greater than the
127-128: Typo in comment.The comment has a typo: "SupplierUnbondigPeriodSessions" should be "SupplierUnbondingPeriodSessions".
- // Update the SupplierUnbondigPeriodSessions such that it is greater than the + // Update the SupplierUnbondingPeriodSessions such that it is greater than the
164-165: Typo in comment.The comment has a typo: "SupplierUnbondigPeriodSessions" should be "SupplierUnbondingPeriodSessions".
- // Update the SupplierUnbondigPeriodSessions such that it is greater than the + // Update the SupplierUnbondingPeriodSessions such that it is greater than the
227-227: Typo in variable name.The variable name has a typo: "expectedSupplierUnbondingPerid" should be "expectedSupplierUnbondingPeriod".
- var expectedSupplierUnbondingPerid int64 = 5 + var expectedSupplierUnbondingPeriod int64 = 5
273-273: Typo in variable name.The variable name has a typo: "newProofWindowCloseBlobcks" should be "newProofWindowCloseBlocks".
- newProofWindowCloseBlobcks := types.GetSessionEndToProofWindowCloseBlocks(params) + deltaBlocks + newProofWindowCloseBlocks := types.GetSessionEndToProofWindowCloseBlocks(params) + deltaBlocks
Summary
This PR adds
supplier_unbonding_periodgovernance param as a follow-up to [Supplier] Implement unbonding period #666.supplier_unbonding_periodresutling blocks to be greater than the cumulated proof window close blocks.sharedParamsvalidation when updating individual params (as some values depend on each other)Example
Supplierunstaking/unbonding timelineSupplier unstaking (UnbondingPeriod = 3 sessioons, NumBlocksPerSession = 10, ProofWindowClose = 15 blocks)
timeline section Session 1 <br> [1,10] Blocks 1 - 10 : Supplier1 is active : Supplier2 is active section Session 2 <br> [11,20] Block 11 : Supplier1 unstakes <br> Still active Block 18 : Supplier2 unstakes <br> Still active Block 20: DO Deactivate Supplier1 : DO Deactivate Supplier2 section Session 3 <br>[21, 30] Block 21: Supplier1 IS NOT included in sessions : Supplier2 IS NOT included in sessions section Session 4 <br>[31, 40] Block 35: ProofWindowClosed section Session 5 <br>[41, 50] Block 50: DO UNBOND Supplier1 : DO UNBOND Supplier2Issue
Type of change
Select one or more:
Testing
Documentation changes (only if making doc changes)
make docusaurus_start; only needed if you make doc changesLocal Testing (only if making code changes)
make go_develop_and_testmake test_e2ePR Testing (only if making code changes)
devnet-test-e2elabel to the PR.make trigger_ciif you want to re-trigger tests without any code changesSanity Checklist
Summary by CodeRabbit
New Features
SupplierUnbondingPeriodparameter, defining the waiting duration for suppliers after unstaking before they can withdraw tokens.SupplierUnbondingPeriodSessionsparameter to enhance the unbonding process functionality.Bug Fixes
SupplierUnbondingPeriodandSupplierUnbondingPeriodSessions.Tests
SupplierUnbondingPeriodSessionswith validation against thresholds.