Add a cli method for vault withdrawal #2369
Conversation
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
WalkthroughThis pull request introduces several changes across multiple files. A new branch trigger Changes
Possibly related PRs
Suggested reviewers
Poem
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
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 and nitpick comments (3)
protocol/x/vault/client/cli/tx.go (1)
248-297: Consider checking name consistencyThe function is named
CmdWithdrawFromVault, but it creates aMsgWithdrawFromMegavaultmessage. This inconsistency might be confusing. Consider aligning the names or adding a comment explaining the difference if it's intentional.protocol/x/listing/keeper/listing.go (1)
Line range hint
1-324: Overall assessment of changes in listing.goThe primary change in this file is the modification of the
atomicResolutioncalculation in theCreatePerpetualfunction. While this change appears to improve precision by accounting for the ticker's decimal places, it's crucial to ensure that this modification aligns with the overall system design and doesn't introduce unintended consequences.Additional considerations:
- Update relevant documentation to reflect this change in atomic resolution calculation.
- Consider adding unit tests specifically for this new calculation, covering various decimal precision scenarios.
- Evaluate if any other parts of the system (e.g., order books, price feeds) need adjustments to accommodate this change.
- Assess the potential impact on gas costs if this calculation is performed frequently.
Given the importance of price resolution in a financial system, consider implementing a comprehensive suite of integration tests that verify the entire lifecycle of a perpetual, from creation to trading and settlement, using this new atomic resolution calculation.
protocol/x/listing/keeper/listing_test.go (1)
188-189: Approved: AtomicResolution adjustment and explanation.The change in the expected
AtomicResolutionvalue from -15 to -5 is consistent with the increase in theDecimalsfield. The added comment clearly explains the calculation:// Expected resolution = -6 - (Floor(log10(1000000000))+10) = -5This adjustment ensures that the test remains accurate with the new precision level.
Consider slightly expanding the comment for even more clarity:
// Expected resolution = -Exponent - (Floor(log10(ReferencePrice)) + Decimals) // = -6 - (Floor(log10(1000000000)) + 10) = -5This expanded comment explicitly shows where each value in the calculation comes from, making it easier for future developers to understand the logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- .github/workflows/protocol-build-and-push.yml (1 hunks)
- protocol/x/listing/keeper/listing.go (1 hunks)
- protocol/x/listing/keeper/listing_test.go (2 hunks)
- protocol/x/vault/client/cli/tx.go (2 hunks)
🔇 Additional comments (6)
.github/workflows/protocol-build-and-push.yml (1)
6-6: Approved, but clarification needed on the purpose of the new branch trigger.The addition of the 'tra617' branch to the workflow trigger looks correct. However, could you please clarify the purpose of this new branch trigger? Is it related to the CLI method for vault withdrawal mentioned in the PR objectives?
To ensure this branch exists and contains relevant changes, please run:
protocol/x/vault/client/cli/tx.go (3)
33-33: LGTM: New command added correctlyThe new
CmdWithdrawFromVaultcommand is correctly added to the list of commands in theGetTxCmdfunction. This addition aligns with the PR objective of introducing a CLI method for vault withdrawal.
248-297: LGTM: New CmdWithdrawFromVault function implemented correctlyThe implementation of
CmdWithdrawFromVaultfollows the established pattern for CLI commands in this module. It correctly handles argument parsing, message creation, and transaction broadcasting.
248-297: Reminder: Add tests for the new commandThe PR objectives mention an empty test plan. As this is a new feature, it's crucial to have comprehensive tests to ensure its correctness and prevent regressions. Please add appropriate unit tests for this new command.
To help with test coverage, you can run the following command to check for existing tests and identify gaps:
protocol/x/listing/keeper/listing.go (1)
151-154: Verify the impact of the atomic resolution calculation change.The modification to the
atomicResolutioncalculation now incorporates the ticker's decimal precision. This change appears to aim for a more accurate representation of the perpetual's price resolution.However, please address the following points:
- Can you provide more context on why this change was necessary?
- How does this affect existing perpetuals, if at all?
- Have you considered any potential edge cases, especially for tickers with extreme decimal values?
- Are there any implications for order placement, matching, or settlement that need to be addressed in other parts of the system?
To ensure this change doesn't introduce any inconsistencies, please run the following verification:
This script will help identify any areas of the codebase that might need adjustment due to this change in atomic resolution calculation.
✅ Verification successful
Atomic resolution calculation change verified successfully.
- No hardcoded atomic resolution values or conflicting constants found.
- Incorporating ticker's decimal precision does not introduce inconsistencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any hardcoded atomic resolution values or assumptions in the codebase that might conflict with this new calculation. # Test: Search for hardcoded atomic resolution values or related constants rg --type go -e 'atomicResolution\s*:=\s*\d+' -e 'const\s+.*Resolution.*=\s*\d+' # Test: Look for any other usage of ResolutionOffset to ensure consistency rg --type go 'ResolutionOffset' # Test: Check for any price-related calculations that might be affected by this change rg --type go -e 'price.*calculation' -e 'calculate.*price'Length of output: 5640
protocol/x/listing/keeper/listing_test.go (1)
155-155: Please clarify the reason for increasing theDecimalsvalue.The
Decimalsfield in theTickerstruct has been changed from 6 to 10. This increases the precision of the currency pair significantly. Could you please explain:
- Why was this change necessary?
- What are the implications of this change on the test cases and the system in general?
- Are there any other parts of the codebase that need to be updated to accommodate this change?
✅ Verification successful
Verification Successful: The change to the
Decimalsfield is isolated tolisting_test.goand does not affect other parts of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of Decimals field in Ticker structs rg --type go 'Ticker\s*{[^}]*Decimals:\s*\d+' .Length of output: 172
Script:
#!/bin/bash # Corrected search for other occurrences of Decimals field in Ticker structs rg --type go 'Ticker\s*\{\s*[^}]*Decimals:\s*\d+' .Length of output: 53
Script:
#!/bin/bash # Using ast-grep to find all Decimals field definitions within Ticker structs ast-grep --lang go --pattern 'Ticker { $$$ Decimals: $_, $$$ }' .Length of output: 67
| // Parse withdrawer owner and number | ||
| withdrawerOwner := args[0] | ||
| withdrawerNumber, err := cast.ToUint32E(args[1]) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Parse shares. | ||
| shares, err := cast.ToUint64E(args[2]) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Parse min quote quantums. | ||
| minQuoteQuantums, err := cast.ToUint64E(args[3]) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding more input validation
While the current implementation checks for type correctness, consider adding more validation for the input values. For example, you might want to ensure that shares and minQuoteQuantums are non-zero, or that withdrawerNumber is within a valid range.
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
protocol/x/vault/client/cli/tx.go (1)
248-297: Overall structure and implementation look goodThe new
CmdWithdrawFromMegavaultfunction is well-structured and consistent with other command implementations in this file. It correctly handles argument parsing, message creation, and transaction broadcasting/generation.Consider enhancing input validation
While the current implementation checks for type correctness, consider adding more validation for the input values:
- Ensure that
sharesandminQuoteQuantumsare non-zero.- Add checks for potential overflow when converting to uint64.
Example implementation:
shares, err := cast.ToUint64E(args[2]) if err != nil { return fmt.Errorf("invalid shares value: %w", err) } if shares == 0 { return fmt.Errorf("shares must be greater than zero") } // Similar checks for minQuoteQuantumsImprove error messages for better user experience
Consider providing more descriptive error messages when parsing fails. This will help users understand and correct their input more easily.
Example:
withdrawerNumber, err := cast.ToUint32E(args[1]) if err != nil { return fmt.Errorf("invalid withdrawer number '%s': %w", args[1], err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- protocol/x/vault/client/cli/tx.go (2 hunks)
🔇 Additional comments (1)
protocol/x/vault/client/cli/tx.go (1)
33-33: LGTM: New command added correctlyThe new
CmdWithdrawFromMegavaultcommand is correctly added to the list of commands in theGetTxCmdfunction. This addition is consistent with the existing pattern and allows the new functionality to be accessible through the CLI.
|
@Mergifyio backport release/protocol/v7.x |
✅ Backports have been created
|
Signed-off-by: Shrenuj Bansal <[email protected]> (cherry picked from commit 53258cb)
Co-authored-by: shrenujb <[email protected]>
|
@Mergifyio backport release/protocol/v7.x |
✅ Backports have been created
|
Signed-off-by: Shrenuj Bansal <[email protected]>
Changelist
Add a cli method for vault withdrawal
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Bug Fixes
atomicResolutionto incorporate decimal precision.Tests