fix: deprecate finalize_arb and make new version of it
#172
Conversation
WalkthroughThe pull request introduces changes to the Changes
Sequence DiagramsequenceDiagram
participant User
participant MiniswapModule
participant PoolState
User->>MiniswapModule: finalize_arb_v2(account, arb_index)
MiniswapModule->>PoolState: Check pool balance
MiniswapModule->>MiniswapModule: Execute finalize_arb_hook
MiniswapModule-->>User: Arbitrage finalization complete
Possibly related PRs
Suggested reviewers
Poem
🪧 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: 0
🧹 Nitpick comments (1)
precompile/modules/initia_stdlib/sources/minitswap.move (1)
1455-1459: New entry function aligns with the updated withdrawal flow.The comments on lines 1455-1457 clarify the rationale behind having the executor manually trigger MsgFinalizeWithdrawal before calling “finalize_arb_v2”. This approach reduces the complexity of the pool’s responsibilities. Ensure that all external references are updated to call this new function instead of the deprecated one.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
precompile/modules/initia_stdlib/sources/minitswap.move(3 hunks)
🔇 Additional comments (5)
precompile/modules/initia_stdlib/sources/minitswap.move (5)
45-45: Good use of a dedicated deprecation error code.
The addition of "EDEPRECATED" is a clear, standardized way to abort deprecated functions and communicate this status to developers.
1468-1469: Consistent retrieval of pool address and global state.
No issues here—we see that you address the pool object properly. It's a safe approach to first fetch the pool's address, then borrow the global resource.
1476-1487: Verification of pool balance is correct but consider further checks.
Verifying that 'uinit_pool_balance' covers 'arb_info.ibc_op_init_sent' prevents underflow. However, consider also checking whether the pool is active before finalizing the arbitrage. If the pool is deactivated after the arbitrage is set up but before this call, it might lead to confusion.
1488-1488: Empty line.
No changes needed here.
1489-1502: Properly deprecates the old finalize_arb function.
Aborting with EDEPRECATED is appropriate to avoid unintended usage. This matches the PR objective of manually calling MsgFinalizeWithdrawal first, then switching to “finalize_arb_v2.”
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
precompile/modules/initia_stdlib/sources/minitswap.move (2)
1477-1480: Consider adding more context to the error messageThe error message for insufficient balance could be more descriptive to help users understand the specific requirement.
Consider updating the error message to include the expected and actual values:
- error::invalid_state(ENOT_ENOUGH_BALANCE) + error::invalid_state_with_data(ENOT_ENOUGH_BALANCE, arb_info.ibc_op_init_sent - uinit_pool_balance)
1489-1502: Enhance deprecation notice with migration guidanceWhile the function is correctly marked as deprecated, it would be helpful to provide migration guidance in the documentation.
Add a more detailed deprecation notice:
- // deprecated + /// @deprecated Use `finalize_arb_v2` instead. + /// This function is deprecated as the execution flow has changed to require + /// executors to trigger MsgFinalizeWithdrawal independently before calling + /// finalize_arb_v2.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
precompile/modules/initia_stdlib/sources/minitswap.move(3 hunks)
🔇 Additional comments (2)
precompile/modules/initia_stdlib/sources/minitswap.move (2)
45-45: LGTM: Error constant for deprecated function
The error constant follows the module's naming convention and is appropriately used for the deprecated function.
1455-1458: LGTM: Well-documented function signature change
The documentation clearly explains the change in execution flow where the executor must now trigger MsgFinalizeWithdrawal independently before calling this function.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
precompile/modules/initia_stdlib/sources/minitswap.move (2)
1455-1458: Add documentation for the new function.The function lacks documentation explaining its purpose, parameters, and return values. Consider adding a doc comment that explains:
- The purpose of the function
- The parameters it accepts
- Any preconditions or requirements
- The expected behavior
+ /// Finalizes an arbitrage operation after MsgFinalizeWithdrawal has been executed. + /// * `account` - The account executing the finalization + /// * `arb_index` - The index of the arbitrage operation to finalize + /// + /// This function verifies that the pool has sufficient balance and triggers the finalization hook. public entry fun finalize_arb_v2(
1489-1502: Enhance deprecation notice with migration guide.While the function is correctly marked as deprecated and aborts with the appropriate error, it would be helpful to provide more context about the deprecation and migration path in the documentation.
- // deprecated + /// @deprecated This function is deprecated and will abort if called. + /// Migration: Use `finalize_arb_v2` instead. The new version requires executing + /// MsgFinalizeWithdrawal independently before calling finalize_arb_v2. + /// See documentation for `finalize_arb_v2` for more details. public entry fun finalize_arb(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
precompile/modules/initia_stdlib/sources/minitswap.move(3 hunks)
🔇 Additional comments (2)
precompile/modules/initia_stdlib/sources/minitswap.move (2)
1476-1480: Verify potential race conditions in balance check.
The balance check between lines 1476-1480 could potentially be subject to race conditions in a concurrent environment. While Move's linear type system provides some protection, it would be good to verify that there are no scenarios where the balance could change between the check and the actual usage.
✅ Verification successful
Let me analyze the code further to verify the potential race conditions.
Based on the code analysis, I can now provide a final response regarding the potential race conditions in the balance check.
Race conditions in balance check are prevented by Move's resource model and acquires annotation.
The balance check at lines 1476-1480 is protected from race conditions by Move's type system and resource model:
- The function containing this code acquires the
ModuleStoreresource, as evidenced by theacquiresannotations throughout the codebase - Move's linear type system ensures that resources can only be accessed by one function at a time
- The balance check is performed within a function that has exclusive access to the pool's resources through the
acquiresmechanism
This means that no other transaction can modify the pool's balance while this check is being performed, effectively preventing race conditions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other functions that might modify the pool balance
# to verify potential race conditions
# Check for functions that modify pool balance
ast-grep --pattern 'fun $name($params) {
$$$
primary_fungible_store::$action($_, $pool_addr, $_, $_)
$$$
}'
# Check for functions that might be called concurrently
rg -A 5 "public entry fun"
Length of output: 106121
Script:
#!/bin/bash
# Search for functions that might modify the pool balance or be called concurrently with the balance check
# Check for functions that read or modify pool balance
ast-grep --pattern 'fun $name($params) {
$$$
coin::balance($pool_addr, $_)
$$$
}'
# Check for functions that might modify pool balance
ast-grep --pattern 'fun $name($params) {
$$$
primary_fungible_store::withdraw($_, $pool_addr, $_)
$$$
}'
# Check for functions that might modify pool balance
ast-grep --pattern 'fun $name($params) {
$$$
primary_fungible_store::deposit($pool_addr, $_)
$$$
}'
# Look for any acquire statements around the balance check
rg -B 5 -A 5 "acquires" precompile/modules/initia_stdlib/sources/minitswap.move
Length of output: 18496
Line range hint 1455-1502: Verify impact of breaking changes on existing integrations.
The change from finalize_arb to finalize_arb_v2 represents a significant workflow change where MsgFinalizeWithdrawal must now be executed independently. This is a breaking change that could affect existing integrations.
✅ Verification successful
The initial search showed only the function definition but we need to verify:
- If there are any other files importing or using this module
- If there are test files that might be affected
- If there are any other references to this functionality
This will help us determine if this is truly a breaking change that affects other parts of the codebase.
The previous search revealed that there's documentation that mentions finalize_arb. We should check this documentation and also look for related arbitrage functions to fully understand the scope of this change and its impact on the ecosystem.
Based on the search results and documentation, I can now generate the final response:
Breaking changes are properly handled with deprecation notice
The changes follow good deprecation practices:
- The old
finalize_arbfunction is marked as deprecated and aborts withEDEPRECATEDerror - The new
finalize_arb_v2function is introduced with clear documentation about the workflow change - The only references to these functions are in the module itself, with no external dependencies found
The code changes represent a clean transition that maintains backward compatibility by keeping the old function while clearly marking it as deprecated. This allows existing integrations to gracefully migrate to the new workflow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for existing usages of finalize_arb to identify affected code
# Search for direct function calls
rg -A 5 "finalize_arb\("
# Search for any tests that might need updating
rg -A 5 "test.*finalize.*arb"
Length of output: 579
Script:
#!/bin/bash
# Let's search for any references to finalize_arb in the entire codebase
rg -l "finalize_arb"
# Also search for any potential imports of the minitswap module
rg "use.*minitswap"
# Look for any potential test files related to arbitrage
fd -e move test.*arb
Length of output: 173
Script:
#!/bin/bash
# Let's check the documentation to understand the impact
cat precompile/modules/initia_stdlib/doc/minitswap.md
# Let's also check if there are any other functions that might be related to arbitrage
rg -A 5 "arb" precompile/modules/initia_stdlib/sources/minitswap.move
Length of output: 65661
Description
Fix
As
MsgFinalizeWithdrawalinterface changed deprecatedfinalize_arband createfinalize_arb_v2Difference between
finalize_arbandfinalize_arb_v2finalize_arb, the pool directly executedMsgFinalizeWithdrawal.finalize_arb_v2, the executor must executeMsgFinalizeWithdrawalthemselves before callingfinalize_arb_v2.Although a mismatch between
MsgFinalizeWithdrawalandarb_infocan occur, it doesn't affect the final result since execution only proceeds if the current balance is greater than or equal toarb_info.ibc_op_init_sent.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 changeReviewers 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.
I have...
Summary by CodeRabbit
New Features
Deprecations