Feat/zapper 2 #333
Feat/zapper 2 #333
Conversation
WalkthroughThe pull request introduces updates to the Changes
Possibly related PRs
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
CodeRabbit Configuration File (
|
|
Code Coverage SummaryDiff against mainResults for commit: dfa75cf Minimum allowed coverage is ♻️ This comment has been updated with latest results |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range and nitpick comments (5)
packages/oraiswap-v3/src/types.ts (1)
275-284: Enhance readability ofZapInResultenum descriptionsConsider rephrasing the descriptions in the
ZapInResultenum for better clarity and conciseness.You might revise the descriptions as follows:
export enum ZapInResult { // In range - InRangeNoRouteThroughSelf = "This zap operation has no swap through this pool and the position is in range so the accuracy is good", + InRangeNoRouteThroughSelf = "No swap through this pool; position in range; accuracy is good", - InRangeHasRouteThroughSelf = "This zap operation has swap through this pool and the position is in range so the accuracy is good", + InRangeHasRouteThroughSelf = "Swap through this pool; position in range; accuracy is good", - InRangeHasRouteThroughSelfMayBecomeOutRange = "This zap operation has swap through this pool and the position is in range but the next tick is out of range so the accuracy is low", + InRangeHasRouteThroughSelfMayBecomeOutRange = "Swap through this pool; position in range but next tick out of range; accuracy is low", // Out of range - OutRangeNoRouteThroughSelf = "This zap operation has no swap through this pool and the position is out of range so the accuracy is good", + OutRangeNoRouteThroughSelf = "No swap through this pool; position out of range; accuracy is good", - OutRangeHasRouteThroughSelf = "This zap operation has swap through this pool and the position is out of range but the next tick is not in range so the accuracy is good", + OutRangeHasRouteThroughSelf = "Swap through this pool; position out of range and next tick also out of range; accuracy is good", - OutRangeHasRouteThroughSelfMayBecomeInRange = "This zap operation has swap through this pool and the position is out of range but the next tick is in range so the accuracy is low" + OutRangeHasRouteThroughSelfMayBecomeInRange = "Swap through this pool; position out of range but next tick in range; accuracy is low" }This enhances readability and makes the descriptions more straightforward.
packages/oraiswap-v3/src/helpers.ts (1)
513-514: Ensure consistent use of optional parameters in function signatureIn the
populateMessageZapInfunction, the optional parameterbuildZapInMessageOptionsis added without a space before the colon, which is inconsistent with the rest of the parameters and could affect readability.Apply this diff to maintain consistent formatting:
export const populateMessageZapIn = ( message: ZapInLiquidityResponse, actualAmountXReceived: SmartRouteResponse, actualAmountYReceived: SmartRouteResponse, sqrtPrice: bigint, poolKey: PoolKey, lowerTick: number, upperTick: number, slippage: number, - buildZapInMessageOptions?:buildZapInMessageOptions + buildZapInMessageOptions?: buildZapInMessageOptions ) => {packages/oraiswap-v3/src/zap-consumer.ts (3)
489-496: Enhance Documentation forprocessZapOutPositionLiquidityParametersThe JSDoc comments for the
processZapOutPositionLiquiditymethod (lines 489-496) lack detailed descriptions for the parameters.Improve the documentation to enhance readability and maintainability:
/** * Processes the zap-out operation for a liquidity position. * @param tokenId - The unique identifier of the position token. * @param owner - The address of the position owner. * @param tokenOut - The token to receive after removing liquidity. * @param zapFee - The fee percentage applied during the zap-out process. * @param slippage - (Optional) The acceptable slippage percentage (default is 1%). * @returns Result of the zap-out operation. */Providing clear documentation helps other developers understand the purpose and usage of the method.
Line range hint
280-481: Refactor Long Method forprocessZapInPositionLiquidityThe
processZapInPositionLiquiditymethod spans over 200 lines (lines 280-481), which makes it lengthy and potentially difficult to maintain.Consider refactoring the method into smaller, reusable private methods that handle specific tasks. For example:
- Extract the calculation logic into a separate method.
- Separate the route finding logic.
- Isolate the simulation steps.
This refactoring will improve the readability and maintainability of your code.
35-53: Remove Redundant CommentsThe extensive comments between lines 35-53 explain the flow and methods of the
ZapConsumerclass. While documentation is valuable, these comments duplicate information that should be in documentation rather than inline comments.Consider removing or reducing these comments and instead rely on clear method names and external documentation. This will declutter the code and make it more readable.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- .github/workflows/publish_oraiswap_v3.yml (1 hunks)
- packages/oraiswap-v3/package.json (1 hunks)
- packages/oraiswap-v3/src/error.ts (1 hunks)
- packages/oraiswap-v3/src/helpers.ts (2 hunks)
- packages/oraiswap-v3/src/main.ts (2 hunks)
- packages/oraiswap-v3/src/types.ts (2 hunks)
- packages/oraiswap-v3/src/zap-consumer.ts (9 hunks)
Files skipped from review due to trivial changes (1)
- packages/oraiswap-v3/package.json
Additional context used
actionlint
.github/workflows/publish_oraiswap_v3.yml
24-24: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
26-26: the runner of "actions/setup-node@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
31-31: workflow command "set-output" was deprecated. use
echo "{name}={value}" >> $GITHUB_OUTPUTinstead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions(deprecated-commands)
Biome
packages/oraiswap-v3/src/zap-consumer.ts
[error] 143-143: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
Additional comments not posted (11)
packages/oraiswap-v3/src/error.ts (2)
7-11: LGTM!The
RouteNoLiquidityerror class is implemented correctly:
- It extends the base
Errorclass.- The constructor sets a specific error message, which is a good practice for custom error classes.
- The error class name is descriptive and conveys the purpose of the error.
This new error class enhances error handling by allowing developers to catch and respond to scenarios where a route is valid but lacks the necessary liquidity for transactions. It improves the robustness and clarity of the codebase, allowing for more precise error handling and debugging.
13-16: LGTM!The
SpamTooManyRequestsErrorerror class is implemented correctly:
- It extends the base
Errorclass.- The constructor sets a specific error message, which is a good practice for custom error classes.
- The error class name is descriptive and conveys the purpose of the error.
This new error class could be useful for rate limiting and managing user interactions with the system. It improves the robustness and clarity of the codebase, allowing for more precise error handling and debugging.
packages/oraiswap-v3/src/main.ts (7)
3-3: LGTM!The import statement for
KWT_CONTRACTis syntactically correct.Ensure that
KWT_CONTRACTis used appropriately in the rest of the file.
8-8: LGTM!The import statement for
USDT_CONTRACTis syntactically correct.
12-12: LGTM!The import statement for
extractAddressandparsePoolKeyis syntactically correct.
28-28: LGTM!The change to directly find the token by its name enhances clarity and reduces complexity.
29-31: LGTM!The changes to directly construct the pool string and fetch the pool information immediately after deriving the pool key improve the flow of data retrieval and simplify the pool selection process.
32-33: LGTM!The assignments of
tickSpacingandcurrentTickare syntactically correct.
35-46: LGTM!The changes to use the fetched pool information, dynamically calculate the amount of input tokens based on the token's decimals, and modify the logging of results enhance flexibility and readability.
packages/oraiswap-v3/src/zap-consumer.ts (2)
443-445: Safeguard Against Division by ZeroAt lines 443-445, the calculation involves division operations that could potentially result in a division by zero if
yPerXorxPriceByYare zero.Add checks to ensure that
yPerXandxPriceByYare not zero before performing the division:if (diffX > this._deviation || diffY > this._deviation) { + if (yPerX.isZero() || xPriceByY.isZero()) { + throw new Error("yPerX or xPriceByY is zero, cannot proceed with division."); + } const deltaX = yAmount.sub(yPerX.mul(xAmount)).div(yPerX.add(xPriceByY)); ... }This prevents runtime errors and ensures the reliability of your calculations.
Likely invalid or redundant comment.
184-187: Review Pool State Update LogicAt lines 184-187, after simulating the swap, you're updating the pool's
sqrt_priceandcurrent_tick_indexby averaging:pool.sqrt_price = ((BigInt(pool.sqrt_price) + BigInt(swapResult.target_sqrt_price)) / 2n).toString(); const tick = getTickAtSqrtPrice(BigInt(pool.sqrt_price), poolKey.fee_tier.tick_spacing); pool.current_tick_index = (pool.current_tick_index + tick) / 2;This averaging may not accurately reflect the pool's state after the swap simulation.
Consider verifying whether the averaging correctly represents the new state of the pool. It might be more appropriate to use the
target_sqrt_priceand corresponding tick directly.Run the following script to compare the calculated tick index:
Ensure that the pool state update logic aligns with the domain requirements.
There was a problem hiding this comment.
Actionable comments posted: 10
Outside diff range and nitpick comments (6)
packages/universal-swap/src/universal-demos/from-oraichain-to-evm.ts (2)
20-26: Remove Commented-Out Code for ClarityThe commented-out code between lines 20 and 26 can clutter the codebase and cause confusion. If this code is no longer needed, consider removing it. If it's meant for future use or reference, add a comment explaining its purpose.
59-61: Simplify Function Invocation Without IIFEWrapping
oraichainToEvm()inside an Immediately Invoked Function Expression (IIFE) is unnecessary here. You can call the async function directly and handle any top-level promises appropriately.Apply this diff to simplify the function call:
-(() => { - return oraichainToEvm(); -})(); + +oraichainToEvm();packages/oraidex-common/src/network.ts (3)
327-334: Include the "type" Property for the "PEPE" TokenFor consistency with other CW20 tokens, consider adding the
"type": "cw20"property to the "PEPE" token definition if applicable.Apply this diff to add the
"type"property:{ coinDenom: "PEPE", coinGeckoId: "pepe", coinMinimalDenom: PEPE_ORAICHAIN_DENOM, + type: "cw20", bridgeTo: ["0x38", "0x01"], coinDecimals: 18, coinImageUrl: "https://assets.coingecko.com/coins/images/29850/standard/pepe-token.jpeg?1696528776" },
335-342: Include the "type" Property for the "CAT" TokenSimilarly, consider adding the
"type": "cw20"property to the "CAT" token definition for consistency if it is a CW20 token.Apply this diff:
{ coinDenom: "CAT", coinMinimalDenom: CAT_ORAICHAIN_DENOM, coinDecimals: 18, + type: "cw20", bridgeTo: ["0x38"], coinGeckoId: "simon-s-cat", coinImageUrl: "https://assets.coingecko.com/coins/images/39765/standard/Simon's_Cat_Logo.png?1724017505" },
987-996: Add "CAT" Token to the Ethereum Network if IntendedCurrently, only the "PEPE" token is added to the Ethereum network. If the "CAT" token is also available on Ethereum, consider adding its definition to ensure users can interact with it on this network.
packages/universal-swap/src/handler.ts (1)
1015-1017: Consider removing commented-out code for cleanlinessThe lines related to network switching for EVM wallets are commented out. If these are no longer needed, it's better to remove them to maintain code clarity and reduce clutter.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (8)
- packages/oraidex-common/package.json (1 hunks)
- packages/oraidex-common/src/constant.ts (3 hunks)
- packages/oraidex-common/src/helper.ts (1 hunks)
- packages/oraidex-common/src/network.ts (6 hunks)
- packages/universal-swap/package.json (1 hunks)
- packages/universal-swap/src/handler.ts (8 hunks)
- packages/universal-swap/src/types.ts (1 hunks)
- packages/universal-swap/src/universal-demos/from-oraichain-to-evm.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/oraidex-common/package.json
Additional comments not posted (16)
packages/universal-swap/package.json (1)
3-3: Version updated to a beta release.The version change from
1.1.5to1.1.5-beta.3indicates a transition to a beta release. This suggests that the package may include features that are still in testing or not yet finalized.Please consider the following when using this beta version:
- Thoroughly test the beta version before using it in production environments.
- Inform dependent projects or packages about the beta release and the potential for instability or breaking changes.
- Document any known issues or limitations of the beta release.
packages/universal-swap/src/types.ts (1)
92-92: LGTM! Remember to handle the new property in the consuming code.The addition of the
isCheckBalanceIbcproperty to theSwapOptionsinterface is a valid change that expands the configuration options for swap operations. It allows for more granular control over the balance check behavior in the context of Inter-Blockchain Communication (IBC).Please ensure that the code consuming the
SwapOptionsinterface is updated to handle this new optional property appropriately. If the property is set totrue, the corresponding balance check logic should be triggered during the swap process.Run the following script to verify the usage of the new property:
Verification successful
shell
#!/bin/bashDescription: Verify the usage of the
isCheckBalanceIbcproperty in the codebase.Test: Search for the property usage in TypeScript files.
rg 'isCheckBalanceIbc' -g '.ts' -g '.tsx' -A 5
--- <!-- [monologue] [task level_1/1 level_2/1] Analyze the results from the latest shell script execution to verify the usage of the `isCheckBalanceIbc` property. [observations] - The `isCheckBalanceIbc` property is used in `handler.ts` and `from-oraichain-to-evm.ts`. - In `handler.ts`, the property is checked within conditional statements to control balance checks. - In `from-oraichain-to-evm.ts`, the property is set to `true` within the `SwapOptions` configuration. - The property is defined in `types.ts` as an optional boolean in the `SwapOptions` interface. [analysis] - The property `isCheckBalanceIbc` has been consistently added and utilized across relevant files. - Its usage in conditional statements indicates that the property is actively controlling logic based on its value. - Setting the property in `from-oraichain-to-evm.ts` ensures that it is being used where `SwapOptions` are configured. - No instances suggest that the property is defined but unused or improperly handled. [actions] - Conclude that the `isCheckBalanceIbc` property is appropriately integrated and handled in the codebase. - Approve the review comment as the verification confirms that the property is being utilized correctly. --> **Verified: `isCheckBalanceIbc` is appropriately handled in the codebase.** The `isCheckBalanceIbc` property is correctly integrated and utilized within the relevant parts of the codebase, ensuring that balance checks in the context of IBC are effectively controlled based on this configuration. </details> <details> <summary>Scripts executed</summary> The following scripts were executed for the analysis: Script: ```shell #!/bin/bash # Description: Verify the usage of the `isCheckBalanceIbc` property in the codebase. # Test: Search for the property usage. Expect: Occurrences in the code that consumes SwapOptions. rg --type typescript -A 5 $'isCheckBalanceIbc'Length of output: 80
Script:
Length of output: 98
Script:
Length of output: 0
Script:
#!/bin/bash # Description: Verify the usage of the `isCheckBalanceIbc` property in the codebase. # Test: Search for the property usage in TypeScript files. rg 'isCheckBalanceIbc' -g '*.ts' -g '*.tsx' -A 5Length of output: 2416
packages/oraidex-common/src/constant.ts (5)
56-56: LGTM!The constant is properly declared and follows the existing naming convention. The value is a valid BSC contract address format.
57-57: LGTM!The constant is properly declared and follows the existing naming convention. The value is a valid BSC contract address format.
72-72: LGTM!The constant is properly declared and follows the existing naming convention. The value is a valid Ethereum contract address format.
100-100: LGTM!The constant is properly declared and follows the existing naming convention. The value follows the format of other Oraichain denominators.
101-101: LGTM!The constant is properly declared and follows the existing naming convention. The value follows the format of other Oraichain denominators.
packages/oraidex-common/src/helper.ts (1)
255-268: Verify the function usage in the codebase.The changes to the
getTokenOnOraichainfunction look good. The function now handles cases where multiple tokens are found for a givencoingeckoIdand uses theisNativeflag to distinguish between native and non-native tokens.Please ensure that all calls to this function have been updated to provide the
isNativeargument where necessary.Run the following script to verify the function usage:
Verification successful
Function usage verified
All calls to
getTokenOnOraichaininclude theisNativeargument as required.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `getTokenOnOraichain` provide the `isNative` argument where necessary. # Test: Search for the function usage. Expect: Occurrences with the `isNative` argument. ast-grep --lang typescript --pattern $'getTokenOnOraichain($_, $_)'Length of output: 182
packages/oraidex-common/src/network.ts (3)
121-123: Confirm Accuracy of CoinGecko IDsEnsure that the
CoinGeckoIdvalues"pepe"and"simon-s-cat"correctly correspond to the intended tokens on CoinGecko to avoid mismatches in token data.
57-62: Verify the Existence and Correctness of Newly Added ConstantsPlease ensure that the constants
PEPE_ORAICHAIN_DENOM,CAT_ORAICHAIN_DENOM,PEPE_ETH_CONTRACT,PEPE_BSC_CONTRACT, andCAT_BSC_CONTRACTare correctly defined and exported in"./constant".Run the following script to verify the definitions:
#!/bin/bash # Description: Verify that the new constants are defined in "./constant.ts" # Test: Search for the constant definitions. Expect: Definitions found for all constants. rg 'export const (PEPE_ORAICHAIN_DENOM|CAT_ORAICHAIN_DENOM|PEPE_ETH_CONTRACT|PEPE_BSC_CONTRACT|CAT_BSC_CONTRACT)' ./packages/oraidex-common/src/constant.ts
663-681: Verify "prefixToken" and "bridgeNetworkIdentifier" PropertiesEnsure that the
"prefixToken"and"bridgeNetworkIdentifier"properties for the "PEPE" and "CAT" tokens are correctly set to maintain consistency and proper bridging functionality.Run the following script to check the properties:
packages/universal-swap/src/handler.ts (5)
140-140: Verify the use of 'true' for 'isNative' parameterIn the condition where
isSpecialChain && isSpecialCoingecko,getTokenOnOraichainis called withisNativeset totrue. Please verify that this is the intended behavior and that it does not affect other tokens.
280-280: Confirm the necessity of 'isNative' set to 'true'When handling specific tokens,
getTokenOnOraichainis invoked withisNativeastrue. Ensure this is required for the tokens in question and won't introduce unexpected behavior.
1258-1258: Verify if 'isNative' parameter is required in this contextThe method
getTokenOnOraichainis called without theisNativeparameter. Ensure that omitting this parameter is appropriate here and that the default behavior aligns with the expected outcome.
1287-1287: Confirm correctness of 'getTokenOnOraichain' usage without 'isNative'Another instance where
getTokenOnOraichainis called without specifyingisNative. Please confirm that this is intentional and does not affect token retrieval.
74-75: Ensure all calls to 'getTokenOnOraichain' are updated with the new parameterThe method signature of
getTokenOnOraichainhas changed to acceptisNative?: boolean. Verify that all calls to this method have been updated accordingly.Verification successful
Verification Successful: All calls to
getTokenOnOraichaininhandler.tsinclude the newisNativeparameterAll instances within
packages/universal-swap/src/handler.tshave been correctly updated to include theisNativeparameter.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to 'getTokenOnOraichain' without the 'isNative' parameter. # Search for function calls to 'getTokenOnOraichain' with only one argument. ast-grep --lang typescript --pattern 'getTokenOnOraichain($_)'Length of output: 2298
packages/universal-swap/src/universal-demos/from-oraichain-to-evm.ts
Outdated
Show resolved
Hide resolved
packages/universal-swap/src/universal-demos/from-oraichain-to-evm.ts
Outdated
Show resolved
Hide resolved
packages/universal-swap/src/universal-demos/from-oraichain-to-evm.ts
Outdated
Show resolved
Hide resolved
packages/universal-swap/src/universal-demos/from-oraichain-to-evm.ts
Outdated
Show resolved
Hide resolved
packages/universal-swap/src/universal-demos/from-oraichain-to-evm.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
packages/oraiswap-v3/tests/wasm-bindgen.spec.ts (3)
340-340: Remove unnecessary console.log statementThe
console.log(res);statement in thepositionToTicktest may clutter the test output. It's generally best to remove such statements unless they're needed for debugging purposes.Apply this diff to remove the console log:
- console.log(res);
443-449: Remove redundant empty test for 'checkTickToSqrtPriceRelationship'The test for
checkTickToSqrtPriceRelationshiphas an empty array of test cases, resulting in no executed tests. Since there is another test for this function with actual test cases, consider removing this redundant empty test to keep the test suite clean.Apply this diff to remove the empty test:
- it.each<[number, number, bigint, boolean]>([])( - "checkTickToSqrtPriceRelationship", - (tick, tickSpacing, sqrtPrice, expected) => { - const res = checkTickToSqrtPriceRelationship(tick, tickSpacing, sqrtPrice); - expect(res).toEqual(expected); - } - );
537-543: Consolidate 'checkTickToSqrtPriceRelationship' testsThere are two separate tests for
checkTickToSqrtPriceRelationship. To improve maintainability, consider consolidating them into a single test block with all relevant test cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (6)
packages/oraiswap-v3/tests/data/incentives-fund-manager.wasmis excluded by!**/*.wasmpackages/oraiswap-v3/tests/data/oraiswap-factory.wasmis excluded by!**/*.wasmpackages/oraiswap-v3/tests/data/oraiswap-mixed-router.wasmis excluded by!**/*.wasmpackages/oraiswap-v3/tests/data/oraiswap-oracle.wasmis excluded by!**/*.wasmpackages/oraiswap-v3/tests/data/oraiswap-pair.wasmis excluded by!**/*.wasmpackages/oraiswap-v3/tests/data/zapper.wasmis excluded by!**/*.wasm
📒 Files selected for processing (2)
- packages/oraiswap-v3/package.json (2 hunks)
- packages/oraiswap-v3/tests/wasm-bindgen.spec.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/oraiswap-v3/package.json
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
packages/oraiswap-v3/src/zap-consumer.ts (3)
Line range hint
195-281: Consider refactoringprocessZapInWithSingleSidefor improved readabilityThe
processZapInWithSingleSidemethod effectively handles zap-in with a single token. However, its complexity and multiple conditional branches make it challenging to understand at a glance.Consider refactoring this method to improve readability:
- Extract the token need determination logic into a separate method:
private determineTokenNeed(pool: Pool, upperTick: number, tokenX: TokenItemType, tokenY: TokenItemType): { tokenNeed: TokenItemType, isTokenX: boolean } { const isTokenX = upperTick >= pool.current_tick_index; return { tokenNeed: isTokenX ? tokenX : tokenY, isTokenX }; }
- Extract the route finding and simulation logic into a separate method:
private async findAndSimulateRoute(tokenIn: TokenItemType, tokenNeed: TokenItemType, amountIn: string, poolKey: PoolKey, pool: Pool): Promise<{ actualReceive: SmartRouteResponse, updatedPool: Pool }> { const actualReceive = await this.findRoute({ sourceAsset: tokenIn, destAsset: tokenNeed, amount: BigInt(amountIn) }); const routes: ActionRoute[] = extractOraidexV3Actions(actualReceive.routes); const updatedPool = await this.simulateSwapOffChainForRoute(routes, poolKey, { ...pool }); return { actualReceive, updatedPool }; }
- Use these extracted methods in the main
processZapInWithSingleSidemethod to simplify its structure.These refactorings will make the method easier to understand and maintain.
Line range hint
284-488: RefactorprocessZapInPositionLiquidityfor improved maintainabilityThe
processZapInPositionLiquiditymethod is comprehensive and handles multiple scenarios for zap-in operations. However, its length and complexity make it challenging to understand and maintain.Consider breaking down this method into smaller, more focused methods:
- Extract the out-of-range handling:
private async handleOutOfRangeZapIn(params: ZapInParams): Promise<ZapInLiquidityResponse> { // Logic for handling out-of-range zap-in }
- Extract the price and liquidity calculations:
private calculatePricesAndLiquidity(pool: Pool, tokenX: TokenItemType, tokenY: TokenItemType, tokenIn: TokenItemType, amountIn: string): CalculationResult { // Price and liquidity calculation logic }
- Extract the route finding and simulation:
private async findAndSimulateRoutes(tokenIn: TokenItemType, tokenX: TokenItemType, tokenY: TokenItemType, amountInToX: bigint, amountInToY: bigint): RouteSimulationResult { // Route finding and simulation logic }
- Use these extracted methods in the main
processZapInPositionLiquiditymethod:public async processZapInPositionLiquidity(params: ZapInParams): Promise<ZapInLiquidityResponse> { // Handle out-of-range case if (params.lowerTick >= params.pool.pool.current_tick_index || params.upperTick < params.pool.pool.current_tick_index) { return this.handleOutOfRangeZapIn(params); } // Calculate prices and liquidity const calculationResult = this.calculatePricesAndLiquidity(params.pool.pool, params.tokenX, params.tokenY, params.tokenIn, params.amountIn); // Find and simulate routes const routeSimulationResult = await this.findAndSimulateRoutes(params.tokenIn, params.tokenX, params.tokenY, calculationResult.amountInToX, calculationResult.amountInToY); // Process results and return response // ... }This refactoring will improve the method's readability and maintainability by breaking it down into more manageable pieces.
Line range hint
492-542: Improve robustness of token finding inprocessZapOutPositionLiquidityThe
processZapOutPositionLiquiditymethod is well-structured and effectively uses helper functions. However, there's a potential issue with finding tokens by address on lines 526 and 531:sourceAsset: oraichainTokens.find((t) => extractAddress(t) === pool.pool_key.token_x) as TokenItemType,This approach assumes that the token will always be found, which may not always be the case.
Consider adding a null check and error handling for cases where the token is not found:
private findTokenByAddress(address: string): TokenItemType { const token = oraichainTokens.find((t) => extractAddress(t) === address); if (!token) { throw new Error(`Token not found for address: ${address}`); } return token; } // In processZapOutPositionLiquidity: const [xRouteInfo, yRouteInfo] = await this.findZapRoutes([ { sourceAsset: this.findTokenByAddress(pool.pool_key.token_x), destAsset: tokenOut, amount: amountX }, { sourceAsset: this.findTokenByAddress(pool.pool_key.token_y), destAsset: tokenOut, amount: amountY } ]);This change will make the code more robust by explicitly handling cases where a token might not be found, preventing potential runtime errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/oraiswap-v3/package.json (2 hunks)
- packages/oraiswap-v3/src/zap-consumer.ts (9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/oraiswap-v3/package.json
🧰 Additional context used
🪛 Biome
packages/oraiswap-v3/src/zap-consumer.ts
[error] 147-147: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
🔇 Additional comments (4)
packages/oraiswap-v3/src/zap-consumer.ts (4)
35-53: Great addition of comprehensive class documentation!The detailed documentation for the
ZapConsumerclass and its methods significantly improves code readability and maintainability. It provides a clear overview of the class's purpose and the functionality of each method, which will be helpful for developers working with this code in the future.
129-136: Efficient implementation offindZapRoutesThe
findZapRoutesmethod is well-implemented, usingPromise.allfor efficient parallel execution of route finding. This approach optimizes performance when finding multiple routes simultaneously.
Line range hint
139-173: Improved precision and simulation accuracy insimulateSwapOffChainThe updates to the
simulateSwapOffChainmethod are commendable:
- The use of BigInt for calculations improves precision, which is crucial for financial operations.
- The new logic for handling liquidity changes enhances the simulation accuracy.
These improvements will lead to more reliable swap simulations.
🧰 Tools
🪛 Biome
[error] 147-147: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
Line range hint
1-542: Overall assessment of ZapConsumer class updatesThe changes to the
ZapConsumerclass significantly enhance its functionality, particularly in handling zap operations and route finding. The additions of methods likefindRoute,findZapRoutes, andsimulateSwapOffChainForRouteprovide more granular control over the zap process.Key improvements:
- Better error handling and more precise calculations using BigInt.
- Comprehensive documentation added to the class and its methods.
- New methods for handling various zap scenarios.
Areas for further improvement:
- Some methods, particularly
processZapInPositionLiquidity, are quite complex and could benefit from further refactoring.- Consider adding more robust error handling, especially when dealing with token lookups.
- Be cautious of potential stack overflow in recursive calls within
findRoute.Overall, these changes represent a significant improvement to the
ZapConsumerclass, enhancing its capabilities and making it more versatile for handling various zap operations.
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (4)
packages/oraiswap-v3/tests/wasm-bindgen.spec.ts (3)
345-348: Add clarifying comment to getGlobalMaxSqrtPrice testThe test for
getGlobalMaxSqrtPricecorrectly checks its equality withgetMaxSqrtPrice(1). However, it would be helpful to add a comment explaining the relationship between these two functions and why they are expected to be equal. This will make the test more self-documenting and easier for other developers to understand.Example:
it("getGlobalMaxSqrtPrice", () => { // getGlobalMaxSqrtPrice should return the maximum sqrt price // which is equivalent to getMaxSqrtPrice with a tick spacing of 1 const res = getGlobalMaxSqrtPrice(); expect(res).toEqual(getMaxSqrtPrice(1)); });
350-373: Add explanatory comments to constant value testsThe tests for
getTickSearchRange,getMaxChunk,getMaxTickCross,getMaxPoolKeysReturned, andgetMaxPoolPairsReturnedcorrectly check for specific constant values. To improve the readability and maintainability of these tests, consider adding brief comments explaining the significance of each constant value.Example:
it("getTickSearchRange", () => { // The tick search range is expected to be 256 ticks // This value determines the range of ticks to search when finding the next initialized tick const res = getTickSearchRange(); expect(res).toEqual(256); }); it.each<[number]>([[1]])("getMaxChunk", (tickSpacing) => { // The maximum chunk value is 6931 for a tick spacing of 1 // This represents the maximum number of chunks in the bitmap for the given tick spacing const res = getMaxChunk(tickSpacing); expect(res).toEqual(6931); }); // Add similar comments for the other constant value testsThese comments will help other developers understand the purpose and importance of these constants in the context of the protocol.
435-445: Add explanatory comment to isTokenX testThe test for
isTokenXincludes multiple scenarios with different token address formats, which is good. To improve the clarity of this test, consider adding a comment explaining the different token address formats and their significance in determining the token order.Example:
it.each<[string, string, boolean]>([ // Test cases with explanatory comments // Case 1: Standard address vs. native token symbol ["orai1zyvk3n9r8sax4xvqph97pxuhduqqsqwq6dwzj2", "orai", false], // Case 2: Standard address vs. factory token address ["orai1zyvk3n9r8sax4xvqph97pxuhduqqsqwq6dwzj2", "factory/abc/ton", false], // Case 3: Native token symbol vs. standard address ["orai", "orai1zyvk3n9r8sax4xvqph97pxuhduqqsqwq6dwzj2", true] ])( "isTokenX", (tokenX, tokenY, expected) => { // isTokenX determines if tokenX comes before tokenY in the canonical ordering // This ordering is crucial for consistent pool creation and interaction const res = isTokenX(tokenX, tokenY); expect(res).toEqual(expected); } );This additional context will help other developers understand the purpose of the
isTokenXfunction and the significance of different token address formats in the protocol.packages/oraiswap-v3/src/main.ts (1)
45-55: Remove timing code around commented-out functionThe
console.time("processZapInPositionLiquidity");andconsole.timeEnd("processZapInPositionLiquidity");calls are surrounding a function that is currently commented out. This results in timing an empty block of code, which may be misleading.Consider commenting out or removing the timing calls, or uncommenting the function if you plan to use it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- packages/oraiswap-v3/package.json (2 hunks)
- packages/oraiswap-v3/src/helpers.ts (2 hunks)
- packages/oraiswap-v3/src/main.ts (2 hunks)
- packages/oraiswap-v3/src/types.ts (2 hunks)
- packages/oraiswap-v3/src/zap-consumer.ts (9 hunks)
- packages/oraiswap-v3/tests/wasm-bindgen.spec.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/oraiswap-v3/package.json
- packages/oraiswap-v3/src/helpers.ts
- packages/oraiswap-v3/src/types.ts
- packages/oraiswap-v3/src/zap-consumer.ts
🔇 Additional comments (2)
packages/oraiswap-v3/tests/wasm-bindgen.spec.ts (1)
533-548: Improve test coverage for getTickAtSqrtPrice and checkTickToSqrtPriceRelationshipBoth the
getTickAtSqrtPriceandcheckTickToSqrtPriceRelationshiptests have two scenarios each, which is a good start. However, to ensure these functions work correctly across a wider range of inputs, consider adding more test cases:For
getTickAtSqrtPrice:
- Add cases with different tick spacings
- Include edge cases (minimum and maximum sqrt prices)
- Test with sqrt prices that correspond to tick boundaries
it.each<[bigint, number, </blockquote></details> <details> <summary>packages/oraiswap-v3/src/main.ts (1)</summary><blockquote> `31-33`: **Initialization of 'tokenIn', 'pool', and 'poolKey' is correct** The variables `tokenIn`, `pool`, and `poolKey` are correctly initialized, setting up the necessary components for further processing. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| describe("wasm bind gen tests", () => { | ||
| it.each< | ||
| [ | ||
| { | ||
| currentSqrtPrice: bigint; | ||
| targetSqrtPrice: bigint; | ||
| liquidity: bigint; | ||
| amount: bigint; | ||
| byAmountIn: boolean; | ||
| fee: bigint; | ||
| }, | ||
| { | ||
| next_sqrt_price: bigint; | ||
| amount_in: bigint; | ||
| amount_out: bigint; | ||
| fee_amount: bigint; | ||
| } | ||
| ] | ||
| >([ | ||
| [ | ||
| { | ||
| currentSqrtPrice: 10n ** 24n, | ||
| targetSqrtPrice: 1004987562112089027021926n, | ||
| liquidity: 2000n * 10n ** 6n, | ||
| amount: 1n, | ||
| byAmountIn: true, | ||
| fee: 6n * 10n ** 4n | ||
| }, | ||
| { | ||
| next_sqrt_price: 10n ** 24n, | ||
| amount_in: 0n, | ||
| amount_out: 0n, | ||
| fee_amount: 1n | ||
| } | ||
| ], | ||
| [ | ||
| { | ||
| currentSqrtPrice: 10n ** 24n, | ||
| targetSqrtPrice: 1004987562112089027021926n, | ||
| liquidity: 2000n * 10n ** 6n, | ||
| amount: 20n, | ||
| byAmountIn: true, | ||
| fee: 6n * 10n ** 4n | ||
| }, | ||
| { | ||
| next_sqrt_price: 1004987562112089027021926n, | ||
| amount_in: 10n, | ||
| amount_out: 9n, | ||
| fee_amount: 1n | ||
| } | ||
| ], | ||
| [ | ||
| { | ||
| currentSqrtPrice: 10n ** 24n, | ||
| targetSqrtPrice: 1004987562112089027021926n, | ||
| liquidity: 2000n * 10n ** 6n, | ||
| amount: 20n, | ||
| byAmountIn: false, | ||
| fee: 6n * 10n ** 4n | ||
| }, | ||
| { | ||
| next_sqrt_price: 1004987562112089027021926n, | ||
| amount_in: 10n, | ||
| amount_out: 9n, | ||
| fee_amount: 1n | ||
| } | ||
| ] | ||
| ])("compute swap step", (input, output) => { | ||
| const res = computeSwapStep( | ||
| input.currentSqrtPrice, | ||
| input.targetSqrtPrice, | ||
| input.liquidity, | ||
| input.amount, | ||
| input.byAmountIn, | ||
| input.fee | ||
| ); | ||
| expect(res).toMatchObject(output); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding more test cases for edge scenarios
The current test cases for computeSwapStep provide a good starting point. However, to ensure robust testing, consider adding more test cases that cover edge scenarios such as:
- Extreme liquidity values (very low or very high)
- Cases where the target price is equal to the current price
- Scenarios with zero amounts or fees
This will help ensure the function behaves correctly under a wider range of conditions.
| it.each< | ||
| [ | ||
| { | ||
| sqrtPriceA: bigint; | ||
| sqrtPriceB: bigint; | ||
| liquidity: bigint; | ||
| roundUp: boolean; | ||
| }, | ||
| bigint | ||
| ] | ||
| >([ | ||
| [ | ||
| { | ||
| sqrtPriceA: 10n ** 24n, | ||
| sqrtPriceB: 10n ** 24n, | ||
| liquidity: 0n, | ||
| roundUp: false | ||
| }, | ||
| 0n | ||
| ], | ||
| [ | ||
| { | ||
| sqrtPriceA: 10n ** 24n, | ||
| sqrtPriceB: 2n * 10n ** 24n, | ||
| liquidity: 2n * 10n ** 6n, | ||
| roundUp: false | ||
| }, | ||
| 1n | ||
| ] | ||
| ])("get delta x", (input, output) => { | ||
| const res = getDeltaX(input.sqrtPriceA, input.sqrtPriceB, input.liquidity, input.roundUp); | ||
| expect(res).toEqual(output); | ||
| }); | ||
|
|
||
| it.each< | ||
| [ | ||
| { | ||
| sqrtPriceA: bigint; | ||
| sqrtPriceB: bigint; | ||
| liquidity: bigint; | ||
| roundUp: boolean; | ||
| }, | ||
| bigint | ||
| ] | ||
| >([ | ||
| [ | ||
| { | ||
| sqrtPriceA: 10n ** 24n, | ||
| sqrtPriceB: 10n ** 24n, | ||
| liquidity: 0n, | ||
| roundUp: false | ||
| }, | ||
| 0n | ||
| ], | ||
| [ | ||
| { | ||
| sqrtPriceA: 10n ** 24n, | ||
| sqrtPriceB: 2n * 10n ** 24n, | ||
| liquidity: 2n * 10n ** 6n, | ||
| roundUp: false | ||
| }, | ||
| 2n | ||
| ] | ||
| ])("get delta y", (input, output) => { | ||
| const res = getDeltaY(input.sqrtPriceA, input.sqrtPriceB, input.liquidity, input.roundUp); | ||
| expect(res).toEqual(output); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for getDeltaX and getDeltaY functions
The current test cases for getDeltaX and getDeltaY cover basic scenarios. To improve the robustness of these tests, consider adding more test cases that include:
- Extreme price differences
- Very large liquidity values
- Cases where sqrtPriceA > sqrtPriceB and vice versa
- Edge cases where the result might be close to overflowing
This will help ensure these functions behave correctly across a wider range of inputs.
| it.each< | ||
| [ | ||
| { | ||
| sqrtPrice: bigint; | ||
| liquidity: bigint; | ||
| amount: bigint; | ||
| xToY: boolean; | ||
| }, | ||
| bigint | ||
| ] | ||
| >([ | ||
| [ | ||
| { | ||
| sqrtPrice: 10n ** 24n, | ||
| liquidity: 10n ** 6n, | ||
| amount: 1n, | ||
| xToY: true | ||
| }, | ||
| 5n * 10n ** 23n | ||
| ] | ||
| ])("get next sqrt price from input", (input, output) => { | ||
| const res = getNextSqrtPriceFromInput(input.sqrtPrice, input.liquidity, input.amount, input.xToY); | ||
| expect(res).toEqual(output); | ||
| }); | ||
|
|
||
| it.each< | ||
| [ | ||
| { | ||
| sqrtPrice: bigint; | ||
| liquidity: bigint; | ||
| amount: bigint; | ||
| xToY: boolean; | ||
| }, | ||
| bigint | ||
| ] | ||
| >([ | ||
| [ | ||
| { | ||
| sqrtPrice: 10n ** 24n, | ||
| liquidity: 2n * 10n ** 6n, | ||
| amount: 1n, | ||
| xToY: true | ||
| }, | ||
| 5n * 10n ** 23n | ||
| ] | ||
| ])("get next sqrt price from output", (input, output) => { | ||
| const res = getNextSqrtPriceFromOutput(input.sqrtPrice, input.liquidity, input.amount, input.xToY); | ||
| expect(res).toEqual(output); | ||
| }); |
There was a problem hiding this comment.
Significantly increase test coverage for price calculation functions
The current test cases for getNextSqrtPriceFromInput and getNextSqrtPriceFromOutput only include one scenario each. This is insufficient for functions that likely have complex behavior. Please add more test cases that cover:
- Both x-to-y and y-to-x directions
- Various liquidity levels (low, medium, high)
- Edge cases with very small or very large amounts
- Scenarios where the price doesn't change
- Cases that might approach the limits of BigInt
Comprehensive testing of these functions is crucial as they are likely core to the swap mechanism.
| it.each< | ||
| [ | ||
| { | ||
| sqrtPrice: bigint; | ||
| liquidity: bigint; | ||
| amount: bigint; | ||
| xToY: boolean; | ||
| }, | ||
| bigint | ||
| ] | ||
| >([ | ||
| [ | ||
| { | ||
| sqrtPrice: 10n ** 24n, | ||
| liquidity: 10n ** 6n, | ||
| amount: 1n, | ||
| xToY: true | ||
| }, | ||
| 5n * 10n ** 23n | ||
| ] | ||
| ])("getNextSqrtPriceXUp", (input, output) => { | ||
| const res = getNextSqrtPriceXUp(input.sqrtPrice, input.liquidity, input.amount, input.xToY); | ||
| expect(res).toEqual(output); | ||
| }); | ||
|
|
||
| it.each< | ||
| [ | ||
| { | ||
| sqrtPrice: bigint; | ||
| liquidity: bigint; | ||
| amount: bigint; | ||
| xToY: boolean; | ||
| }, | ||
| bigint | ||
| ] | ||
| >([ | ||
| [ | ||
| { | ||
| sqrtPrice: 10n ** 24n, | ||
| liquidity: 10n ** 6n, | ||
| amount: 1n, | ||
| xToY: true | ||
| }, | ||
| 2n * 10n ** 24n | ||
| ] | ||
| ])("getNextSqrtPriceYDown", (input, output) => { | ||
| const res = getNextSqrtPriceYDown(input.sqrtPrice, input.liquidity, input.amount, input.xToY); | ||
| expect(res).toEqual(output); | ||
| }); |
There was a problem hiding this comment.
Expand test coverage for getNextSqrtPriceXUp and getNextSqrtPriceYDown
The current test cases for getNextSqrtPriceXUp and getNextSqrtPriceYDown only include one scenario each. To ensure these functions work correctly across various situations, please add more test cases that cover:
- Both x-to-y and y-to-x directions
- Different liquidity levels
- Edge cases with minimal price changes
- Scenarios with large price changes
- Cases where the amount is very small or very large
Thorough testing of these functions is important as they likely play a crucial role in price calculations during swaps.
| it.each< | ||
| [ | ||
| { | ||
| amount: bigint; | ||
| startSqrtPrice: bigint; | ||
| liquidity: bigint; | ||
| fee: bigint; | ||
| byAmountIn: boolean; | ||
| xToY: boolean; | ||
| }, | ||
| boolean | ||
| ] | ||
| >([ | ||
| [ | ||
| { | ||
| amount: 340282366920938463463374607431768211455n, | ||
| startSqrtPrice: 65535383934512647000000000000n, | ||
| liquidity: 0n, | ||
| fee: 1000000000000n, | ||
| byAmountIn: false, | ||
| xToY: false | ||
| }, | ||
| true | ||
| ] | ||
| ])("isEnoughAmountToChangePrice", (input, output) => { | ||
| const res = isEnoughAmountToChangePrice( | ||
| input.amount, | ||
| input.startSqrtPrice, | ||
| input.liquidity, | ||
| input.fee, | ||
| input.byAmountIn, | ||
| input.xToY | ||
| ); | ||
| expect(res).toEqual(output); | ||
| }); |
There was a problem hiding this comment.
Improve test coverage for isEnoughAmountToChangePrice
The current test case for isEnoughAmountToChangePrice only covers one scenario with the maximum BigInt value. While testing with extreme values is good, it's important to have a more comprehensive set of test cases. Consider adding tests that cover:
- Both true and false outcomes
- Various combinations of amount, liquidity, and fee values
- Edge cases where the amount is just enough or not quite enough to change the price
- Scenarios for both byAmountIn true and false
- Cases for both xToY true and false
This will ensure the function behaves correctly across different scenarios.
| it.each< | ||
| [ | ||
| { | ||
| lower_tick_index: number; | ||
| lower_tick_fee_growth_outside_x: bigint; | ||
| lower_tick_fee_growth_outside_y: bigint; | ||
| upper_tick_index: number; | ||
| upper_tick_fee_growth_outside_x: bigint; | ||
| upper_tick_fee_growth_outside_y: bigint; | ||
| pool_current_tick_index: number; | ||
| pool_fee_growth_global_x: bigint; | ||
| pool_fee_growth_global_y: bigint; | ||
| position_fee_growth_inside_x: bigint; | ||
| position_fee_growth_inside_y: bigint; | ||
| position_liquidity: bigint; | ||
| }, | ||
| { | ||
| x: bigint; | ||
| y: bigint; | ||
| } | ||
| ] | ||
| >([ | ||
| [ | ||
| { | ||
| lower_tick_index: -24300, | ||
| lower_tick_fee_growth_outside_x: 30566305483401951213259107n, | ||
| lower_tick_fee_growth_outside_y: 3090193022255581920240205n, | ||
| upper_tick_index: -23900, | ||
| upper_tick_fee_growth_outside_x: 0n, | ||
| upper_tick_fee_growth_outside_y: 0n, | ||
| pool_current_tick_index: -24200, | ||
| pool_fee_growth_global_x: 34015516218039756676745948n, | ||
| pool_fee_growth_global_y: 3360651078360214052633596n, | ||
| position_fee_growth_inside_x: 1856777541687032563592895n, | ||
| position_fee_growth_inside_y: 164732622916975273061067n, | ||
| position_liquidity: 7823906503624803n | ||
| }, | ||
| { | ||
| x: 1245904n, | ||
| y: 82718n | ||
| } | ||
| ] | ||
| ])("calculateFee", (input, output) => { | ||
| const res = calculateFee( | ||
| input.lower_tick_index, | ||
| input.lower_tick_fee_growth_outside_x, | ||
| input.lower_tick_fee_growth_outside_y, | ||
| input.upper_tick_index, | ||
| input.upper_tick_fee_growth_outside_x, | ||
| input.upper_tick_fee_growth_outside_y, | ||
| input.pool_current_tick_index, | ||
| input.pool_fee_growth_global_x, | ||
| input.pool_fee_growth_global_y, | ||
| input.position_fee_growth_inside_x, | ||
| input.position_fee_growth_inside_y, | ||
| input.position_liquidity | ||
| ); | ||
| expect(res).toMatchObject(output); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for calculateFee function
The current test for calculateFee includes a single, complex scenario. While this is a good start, for a function with this many parameters and potential edge cases, it would be beneficial to include more test cases. Consider adding tests that cover:
- Different combinations of tick indices (lower, upper, and current)
- Various fee growth values, including edge cases (very small and very large values)
- Different liquidity amounts
- Scenarios where the current tick is outside the range (below lower and above upper)
- Cases where fee growth values are zero
Example:
it.each<
[
{
lower_tick_index: number;
lower_tick_fee_growth_outside_x: bigint;
lower_tick_fee_growth_outside_y: bigint;
upper_tick_index: number;
upper_tick_fee_growth_outside_x: bigint;
upper_tick_fee_growth_outside_y: bigint;
pool_current_tick_index: number;
pool_fee_growth_global_x: bigint;
pool_fee_growth_global_y: bigint;
position_fee_growth_inside_x: bigint;
position_fee_growth_inside_y: bigint;
position_liquidity: bigint;
},
{
x: bigint;
y: bigint;
}
]
>([
// Existing test case
[
{
lower_tick_index: -24300,
lower_tick_fee_growth_outside_x: 30566305483401951213259107n,
lower_tick_fee_growth_outside_y: 3090193022255581920240205n,
upper_tick_index: -23900,
upper_tick_fee_growth_outside_x: 0n,
upper_tick_fee_growth_outside_y: 0n,
pool_current_tick_index: -24200,
pool_fee_growth_global_x: 34015516218039756676745948n,
pool_fee_growth_global_y: 3360651078360214052633596n,
position_fee_growth_inside_x: 1856777541687032563592895n,
position_fee_growth_inside_y: 164732622916975273061067n,
position_liquidity: 7823906503624803n
},
{
x: 1245904n,
y: 82718n
}
],
// Add more test cases here, covering different scenarios
])("calculateFee", (input, output) => {
const res = calculateFee(
input.lower_tick_index,
input.lower_tick_fee_growth_outside_x,
input.lower_tick_fee_growth_outside_y,
input.upper_tick_index,
input.upper_tick_fee_growth_outside_x,
input.upper_tick_fee_growth_outside_y,
input.pool_current_tick_index,
input.pool_fee_growth_global_x,
input.pool_fee_growth_global_y,
input.position_fee_growth_inside_x,
input.position_fee_growth_inside_y,
input.position_liquidity
);
expect(res).toMatchObject(output);
});This will provide more comprehensive testing for the calculateFee function and help ensure it behaves correctly across a wide range of inputs.
| it.each<[number, number, bigint, boolean]>([])( | ||
| "checkTickToSqrtPriceRelationship", | ||
| (tick, tickSpacing, sqrtPrice, expected) => { | ||
| const res = checkTickToSqrtPriceRelationship(tick, tickSpacing, sqrtPrice); | ||
| expect(res).toEqual(expected); | ||
| } | ||
| ); |
There was a problem hiding this comment.
Implement test cases for checkTickToSqrtPriceRelationship
The current test for checkTickToSqrtPriceRelationship is set up correctly, but it lacks any actual test cases. This means the function is not being tested at all. To properly test this function:
- Add multiple test cases to cover various scenarios.
- Include cases for both true and false outcomes.
- Test with different tick spacings.
- Include edge cases if applicable.
Example:
it.each<[number, number, bigint, boolean]>([
// tick, tickSpacing, sqrtPrice, expected
[0, 1, 1000000000000000000000000n, true],
[1, 1, 1000050000000000000000000n, true],
[-1, 1, 999950000000000000000000n, true],
[0, 2, 1000000000000000000000000n, true],
[1, 2, 1000100000000000000000000n, false],
// Add more test cases to cover different scenarios
])("checkTickToSqrtPriceRelationship", (tick, tickSpacing, sqrtPrice, expected) => {
const res = checkTickToSqrtPriceRelationship(tick, tickSpacing, sqrtPrice);
expect(res).toEqual(expected);
});Implementing these test cases will ensure that the checkTickToSqrtPriceRelationship function is properly tested and behaves as expected across different inputs.
| it.each< | ||
| [ | ||
| { | ||
| x: bigint; | ||
| lower_tick: number; | ||
| upper_tick: number; | ||
| current_sqrt_price: bigint; | ||
| rounding_up: boolean; | ||
| }, | ||
| { | ||
| amount: bigint; | ||
| l: bigint; | ||
| } | ||
| ] | ||
| >([ | ||
| [ | ||
| { | ||
| x: 1000000n, | ||
| lower_tick: -10, | ||
| upper_tick: 10, | ||
| current_sqrt_price: 10n ** 24n, | ||
| rounding_up: true | ||
| }, | ||
| { | ||
| amount: 1000000n, | ||
| l: 2000600039999999n | ||
| } | ||
| ] | ||
| ])("getLiquidityByX", (input, output) => { | ||
| const res = getLiquidityByX( | ||
| input.x, | ||
| input.lower_tick, | ||
| input.upper_tick, | ||
| input.current_sqrt_price, | ||
| input.rounding_up | ||
| ); | ||
| expect(res).toMatchObject(output); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expand test coverage for getLiquidityByX function
The current test for getLiquidityByX includes a single scenario, which is a good start but may not be sufficient for a function with multiple parameters and potential edge cases. To improve the test coverage:
- Add more test cases with different combinations of input parameters.
- Include edge cases such as:
- Minimum and maximum possible tick values
- Very small and very large x values
- Cases where lower_tick == upper_tick
- Different current_sqrt_price values (below range, within range, above range)
- Test both rounding_up true and false scenarios
Example:
it.each<
[
{
x: bigint;
lower_tick: number;
upper_tick: number;
current_sqrt_price: bigint;
rounding_up: boolean;
},
{
amount: bigint;
l: bigint;
}
]
>([
// Existing test case
[
{
x: 1000000n,
lower_tick: -10,
upper_tick: 10,
current_sqrt_price: 10n ** 24n,
rounding_up: true
},
{
amount: 1000000n,
l: 2000600039999999n
}
],
// Additional test cases
[
{
x: 1n,
lower_tick: -100,
upper_tick: 100,
current_sqrt_price: 10n ** 24n,
rounding_up: false
},
{
amount: 1n,
l: /* expected liquidity */
}
],
// Add more test cases here
])("getLiquidityByX", (input, output) => {
const res = getLiquidityByX(
input.x,
input.lower_tick,
input.upper_tick,
input.current_sqrt_price,
input.rounding_up
);
expect(res).toMatchObject(output);
});By adding these additional test cases, you'll ensure that getLiquidityByX functions correctly across a wide range of inputs and edge cases.
| it.each< | ||
| [ | ||
| { | ||
| y: bigint; | ||
| lower_tick: number; | ||
| upper_tick: number; | ||
| current_sqrt_price: bigint; | ||
| rounding_up: boolean; | ||
| }, | ||
| { | ||
| amount: bigint; | ||
| l: bigint; | ||
| } | ||
| ] | ||
| >([ | ||
| [ | ||
| { | ||
| y: 1000000n, | ||
| lower_tick: -10, | ||
| upper_tick: 10, | ||
| current_sqrt_price: 10n ** 24n, | ||
| rounding_up: true | ||
| }, | ||
| { | ||
| amount: 1000000n, | ||
| l: 2000600039969988n | ||
| } | ||
| ] | ||
| ])("getLiquidityByY", (input, output) => { | ||
| const res = getLiquidityByY( | ||
| input.y, | ||
| input.lower_tick, | ||
| input.upper_tick, | ||
| input.current_sqrt_price, | ||
| input.rounding_up | ||
| ); | ||
| expect(res).toMatchObject(output); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for getLiquidityByY function
The current test for getLiquidityByY includes only one scenario, which is insufficient for a function with multiple parameters and potential edge cases. To improve the test coverage:
- Add more test cases with different combinations of input parameters.
- Include edge cases such as:
- Minimum and maximum possible tick values
- Very small and very large y values
- Cases where lower_tick == upper_tick
- Different current_sqrt_price values (below range, within range, above range)
- Test both rounding_up true and false scenarios
Example:
it.each<
[
{
y: bigint;
lower_tick: number;
upper_tick: number;
current_sqrt_price: bigint;
rounding_up: boolean;
},
{
amount: bigint;
l: bigint;
}
]
>([
// Existing test case
[
{
y: 1000000n,
lower_tick: -10,
upper_tick: 10,
current_sqrt_price: 10n ** 24n,
rounding_up: true
},
{
amount: 1000000n,
l: 2000600039969988n
}
],
// Additional test cases
[
{
y: 1n,
lower_tick: -100,
upper_tick: 100,
current_sqrt_price: 10n ** 24n,
rounding_up: false
},
{
amount: 1n,
l: /* expected liquidity */
}
],
// Add more test cases here
])("getLiquidityByY", (input, output) => {
const res = getLiquidityByY(
input.y,
input.lower_tick,
input.upper_tick,
input.current_sqrt_price,
input.rounding_up
);
expect(res).toMatchObject(output);
});By adding these additional test cases, you'll ensure that getLiquidityByY functions correctly across a wide range of inputs and edge cases.
packages/oraiswap-v3/src/main.ts
Outdated
| // const res = await zapper.processZapInPositionLiquidity({ | ||
| // pool: poolInfo, | ||
| // tokenIn: tokenIn as TokenItemType, | ||
| // amountIn: (10 ** tokenIn.decimals).toString(), | ||
| // lowerTick: currentTick - tickSpacing * 3, | ||
| // upperTick: currentTick + tickSpacing * 3, | ||
| // tokenX: oraichainTokens.find((t) => extractAddress(t) === poolKey.token_x) as TokenItemType, | ||
| // tokenY: oraichainTokens.find((t) => extractAddress(t) === poolKey.token_y) as TokenItemType, | ||
| // }); |
There was a problem hiding this comment.
Ensure 'poolInfo' is defined before use in 'processZapInPositionLiquidity'
The function call to zapper.processZapInPositionLiquidity uses poolInfo, but poolInfo is not defined in the current scope. This will cause a reference error when the code is executed.
Apply this diff to define poolInfo before using it:
+ const poolInfo = await zapper.handler.getPool(poolKey);
const res = await zapper.processZapInPositionLiquidity({
pool: poolInfo,Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (3)
packages/contracts-sdk/package.json (1)
3-3: Version bump approved.The package version has been correctly updated from
1.0.52to1.0.53-beta.1, indicating a new beta release. This change follows semantic versioning practices.Remember to update the CHANGELOG.md file (if one exists) to document the changes introduced in this beta version. This helps users understand what new features or fixes they can expect in this release.
packages/contracts-sdk/src/index.ts (1)
35-36: LGTM! Consider grouping related exports.The new export statements for
IncentivesFundManagerare consistent with the existing pattern in the file and expand the SDK's functionality. Good job!For improved readability and maintainability, consider grouping related exports together. You could move these new exports next to other similar modules or create a new section for fund management related exports if there are more to come in the future.
packages/contracts-sdk/src/IncentivesFundManager.client.ts (1)
14-29: Enhance Documentation with JSDoc CommentsTo improve maintainability and provide better context for other developers, consider adding JSDoc comments to your classes and methods. Documenting parameter types, return types, and method purposes can greatly aid in understanding and using the code effectively.
Also applies to: 48-90
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/contracts-sdk/package.json (1 hunks)
- packages/contracts-sdk/src/IncentivesFundManager.client.ts (1 hunks)
- packages/contracts-sdk/src/IncentivesFundManager.types.ts (1 hunks)
- packages/contracts-sdk/src/index.ts (1 hunks)
🧰 Additional context used
🪛 Biome
packages/contracts-sdk/src/IncentivesFundManager.types.ts
[error] 34-34: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 32-32: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (8)
packages/contracts-sdk/src/IncentivesFundManager.types.ts (7)
1-1: LGTM: Clear and concise type alias for addresses.The
Addrtype alias is well-defined and follows good practices for improving code readability and maintainability.
2-5: LGTM: Well-structured instantiation message interface.The
InstantiateMsginterface is properly defined with clear property types. The optionalownerproperty is correctly typed asAddr | null.
6-16: LGTM: Well-structured execute message type.The
ExecuteMsgtype is properly defined as a union type, representing different possible message structures. The use of optional properties withAddr | nulltype in theupdate_configvariant is correct and follows best practices.
17-17: LGTM: Appropriate type alias for large integers.The
Uint128type alias as a string is a good practice for representing large integers in blockchain contexts, improving code clarity and type safety.
18-26: LGTM: Well-structured asset information type.The
AssetInfotype is properly defined as a union type, representing different asset types (token and native token). This structure allows for clear distinction between asset types and follows good practices for type definitions in TypeScript.
27-30: LGTM: Clear and concise asset interface.The
Assetinterface is well-defined, combining theamount(of typeUint128) with theinfo(of typeAssetInfo). This structure provides a complete representation of an asset.
35-38: LGTM: Well-defined configuration response interface.The
ConfigResponseinterface is properly structured, clearly defining the expected properties of a configuration response. The use of theAddrtype for both properties ensures type consistency.packages/contracts-sdk/src/IncentivesFundManager.client.ts (1)
1-5: Note on Auto-Generated FileThis file is auto-generated by
@oraichain/[email protected]. Please refrain from manually modifying it. If changes are necessary, update the source JSONSchema file and regenerate this file to ensure consistency and maintainability.
| export type QueryMsg = { | ||
| config: {}; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refining the QueryMsg type definition.
The current definition of QueryMsg uses an empty object {} as a type, which is discouraged. Consider refining this type to be more explicit about its structure.
Here's a suggested improvement:
export type QueryMsg = {
config: Record<string, never>;
};This change maintains the same functionality while addressing the static analysis warning about using {} as a type.
🧰 Tools
🪛 Biome
[error] 32-32: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
| export type QueryMsg = { | ||
| config: {}; | ||
| }; | ||
| export interface MigrateMsg {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine the MigrateMsg definition.
The MigrateMsg is currently defined as an empty interface, which is equivalent to {}. This has been flagged by the static analysis tool.
Consider changing it to a type alias if no properties are needed:
export type MigrateMsg = Record<string, never>;This change maintains the same functionality while addressing the static analysis warning about empty interfaces.
🧰 Tools
🪛 Biome
[error] 34-34: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
| this.config = this.config.bind(this); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Unnecessary Binding of Methods
Since you're using arrow functions for your methods, explicit binding in the constructor is unnecessary. Arrow functions lexically bind this, so you can safely remove these bindings to simplify the constructor.
Apply this diff to remove the unnecessary bindings:
- this.config = this.config.bind(this);- this.updateConfig = this.updateConfig.bind(this);
- this.sendFund = this.sendFund.bind(this);Also applies to: 58-59
| config = async (): Promise<ConfigResponse> => { | ||
| return this.client.queryContractSmart(this.contractAddress, { | ||
| config: {} | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Add Error Handling in the config Method
The config method performs a smart contract query but lacks error handling. To enhance robustness, consider wrapping the query in a try-catch block to handle potential exceptions, such as network issues or unexpected responses.
Apply this diff to implement error handling:
config = async (): Promise<ConfigResponse> => {
+ try {
return this.client.queryContractSmart(this.contractAddress, {
config: {}
});
+ } catch (error) {
+ // Handle the error appropriately
+ throw new Error(`Failed to fetch config: ${error.message}`);
+ }
};📝 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.
| config = async (): Promise<ConfigResponse> => { | |
| return this.client.queryContractSmart(this.contractAddress, { | |
| config: {} | |
| }); | |
| }; | |
| config = async (): Promise<ConfigResponse> => { | |
| try { | |
| return this.client.queryContractSmart(this.contractAddress, { | |
| config: {} | |
| }); | |
| } catch (error) { | |
| // Handle the error appropriately | |
| throw new Error(`Failed to fetch config: ${error.message}`); | |
| } | |
| }; |
| client: SigningCosmWasmClient; | ||
| sender: string; | ||
| contractAddress: string; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove Redundant Assignment of contractAddress
In the IncentivesFundManagerClient constructor, this.contractAddress is assigned twice—once in the superclass and again in the subclass. This redundancy is unnecessary and can be removed to streamline the code.
Apply this diff to eliminate the redundant assignment:
constructor(client: SigningCosmWasmClient, sender: string, contractAddress: string) {
super(client, contractAddress);
this.client = client;
this.sender = sender;
- this.contractAddress = contractAddress;
this.updateConfig = this.updateConfig.bind(this);
this.sendFund = this.sendFund.bind(this);
}Committable suggestion was skipped due to low confidence.
| return await this.client.execute(this.sender, this.contractAddress, { | ||
| send_fund: { | ||
| asset, | ||
| receiver | ||
| } | ||
| }, _fee, _memo, _funds); | ||
| }; |
There was a problem hiding this comment.
Handle Execution Errors in sendFund Method
Similar to updateConfig, the sendFund method should include error handling to manage potential execution errors. This ensures that any issues during the transaction are properly caught and communicated.
Apply this diff to add error handling:
sendFund = async ({
asset,
receiver
}: {
asset: Asset;
receiver: Addr;
}, _fee: number | StdFee | "auto" = "auto", _memo?: string, _funds?: Coin[]): Promise<ExecuteResult> => {
+ try {
return await this.client.execute(this.sender, this.contractAddress, {
send_fund: {
asset,
receiver
}
}, _fee, _memo, _funds);
+ } catch (error) {
+ // Handle the error appropriately
+ throw new Error(`Failed to execute sendFund: ${error.message}`);
+ }
};📝 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.
| return await this.client.execute(this.sender, this.contractAddress, { | |
| send_fund: { | |
| asset, | |
| receiver | |
| } | |
| }, _fee, _memo, _funds); | |
| }; | |
| sendFund = async ({ | |
| asset, | |
| receiver | |
| }: { | |
| asset: Asset; | |
| receiver: Addr; | |
| }, _fee: number | StdFee | "auto" = "auto", _memo?: string, _funds?: Coin[]): Promise<ExecuteResult> => { | |
| try { | |
| return await this.client.execute(this.sender, this.contractAddress, { | |
| send_fund: { | |
| asset, | |
| receiver | |
| } | |
| }, _fee, _memo, _funds); | |
| } catch (error) { | |
| // Handle the error appropriately | |
| throw new Error(`Failed to execute sendFund: ${error.message}`); | |
| } | |
| }; |
| return await this.client.execute(this.sender, this.contractAddress, { | ||
| update_config: { | ||
| oraiswap_v3: oraiswapV3, | ||
| owner | ||
| } | ||
| }, _fee, _memo, _funds); | ||
| }; |
There was a problem hiding this comment.
Handle Execution Errors in updateConfig Method
The updateConfig method may encounter execution failures, such as insufficient funds or unauthorized access. Implement error handling to catch and manage these exceptions, providing informative feedback to the user or calling function.
Apply this diff to add error handling:
updateConfig = async ({
oraiswapV3,
owner
}: {
oraiswapV3?: Addr;
owner?: Addr;
}, _fee: number | StdFee | "auto" = "auto", _memo?: string, _funds?: Coin[]): Promise<ExecuteResult> => {
+ try {
return await this.client.execute(this.sender, this.contractAddress, {
update_config: {
oraiswap_v3: oraiswapV3,
owner
}
}, _fee, _memo, _funds);
+ } catch (error) {
+ // Handle the error appropriately
+ throw new Error(`Failed to execute updateConfig: ${error.message}`);
+ }
};📝 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.
| return await this.client.execute(this.sender, this.contractAddress, { | |
| update_config: { | |
| oraiswap_v3: oraiswapV3, | |
| owner | |
| } | |
| }, _fee, _memo, _funds); | |
| }; | |
| updateConfig = async ({ | |
| oraiswapV3, | |
| owner | |
| }: { | |
| oraiswapV3?: Addr; | |
| owner?: Addr; | |
| }, _fee: number | StdFee | "auto" = "auto", _memo?: string, _funds?: Coin[]): Promise<ExecuteResult> => { | |
| try { | |
| return await this.client.execute(this.sender, this.contractAddress, { | |
| update_config: { | |
| oraiswap_v3: oraiswapV3, | |
| owner | |
| } | |
| }, _fee, _memo, _funds); | |
| } catch (error) { | |
| // Handle the error appropriately | |
| throw new Error(`Failed to execute updateConfig: ${error.message}`); | |
| } | |
| }; |
| export interface IncentivesFundManagerInterface extends IncentivesFundManagerReadOnlyInterface { | ||
| contractAddress: string; | ||
| sender: string; | ||
| updateConfig: ({ | ||
| oraiswapV3, | ||
| owner | ||
| }: { | ||
| oraiswapV3?: Addr; | ||
| owner?: Addr; | ||
| }, _fee?: number | StdFee | "auto", _memo?: string, _funds?: Coin[]) => Promise<ExecuteResult>; |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent Usage of oraiswapV3 Parameter Names
The parameter oraiswapV3 is inconsistently used as oraiswap_v3 in the following files:
packages/contracts-sdk/src/IncentivesFundManager.client.tspackages/contracts-sdk/src/OraiswapMixedRouter.client.ts
Please update these instances to oraiswapV3 to maintain consistent naming conventions across interfaces and implementations.
🔗 Analysis chain
Verify Consistency of Parameter Naming
Ensure that parameter names are consistent across your interfaces and methods. For example, check that oraiswapV3 and owner are used uniformly throughout the codebase, adhering to naming conventions.
Run the following script to check for parameter naming consistency:
Also applies to: 62-68, 76-82
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of parameter names 'oraiswapV3' and 'owner'.
# Search for 'oraiswapV3' occurrences
rg --type ts 'oraiswapV3[^A-Za-z0-9_]'
# Search for 'owner' occurrences
rg --type ts 'owner[^A-Za-z0-9_]'
# Ensure that the parameter names match across interfaces and implementations.
Length of output: 29938
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
packages/contracts-sdk/src/OraiswapV3.types.ts (2)
1-1: LGTM. Consider adding a comment for clarity.The addition of
incentives_fund_managerto theInstantiateMsginterface is a good enhancement for managing incentives.Consider adding a brief comment explaining the purpose of the
incentives_fund_manager:export interface InstantiateMsg { // Address of the manager responsible for handling incentives funds incentives_fund_manager: Addr; protocol_fee: Percentage; }Also applies to: 4-4
Line range hint
1-185: Consider adding documentation for the new incentives fund manager feature.The additions related to the incentives fund manager are good enhancements to the contract's functionality. However, to improve maintainability and ease of use for other developers, consider adding more documentation.
Suggestions:
- Add a comment at the top of the file explaining the purpose of the incentives fund manager.
- If there's a README or documentation file for this contract, update it to include information about this new feature and how it should be used.
Example comment for the top of the file:
/** * @file OraiswapV3.types.ts * This file contains type definitions for the OraiswapV3 contract. * It includes support for an incentives fund manager, which is responsible for... * [Add a brief explanation of the incentives fund manager's role and how it's used] */packages/contracts-sdk/src/OraiswapV3.client.ts (2)
248-252: Implementation of the new methodThe
incentivesFundManagermethod is correctly implemented in theOraiswapV3QueryClientclass. It follows the established pattern for querying the smart contract.However, to improve code consistency and maintainability, consider adding a JSDoc comment to describe the method's purpose and return type. For example:
/** * Queries the contract for the address of the incentives fund manager. * @returns {Promise<Addr>} The address of the incentives fund manager. */ incentivesFundManager = async (): Promise<Addr> => { // ... existing implementation ... };
Line range hint
1-1024: Overall structure and impactThe addition of the
incentivesFundManagermethod is well-integrated into the existing codebase. It follows the established patterns and conventions used throughout the file. The change appears to be part of expanding the functionality to interact with an incentives fund manager in the OraiswapV3 contract.To ensure the change is fully integrated, consider the following:
- Update any relevant documentation or comments that describe the
OraiswapV3ReadOnlyInterfaceto include this new method.- If there are any test files associated with this client, make sure to add tests for the new
incentivesFundManagermethod.- Check if any other parts of the codebase need to be updated to use this new method where appropriate.
🧰 Tools
🪛 Biome
[error] 9-9: Do not shadow the global "Boolean" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/contracts-sdk/package.json (1 hunks)
- packages/contracts-sdk/src/OraiswapV3.client.ts (3 hunks)
- packages/contracts-sdk/src/OraiswapV3.types.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/contracts-sdk/package.json
🧰 Additional context used
🪛 Biome
packages/contracts-sdk/src/OraiswapV3.client.ts
[error] 9-9: Do not shadow the global "Boolean" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
packages/contracts-sdk/src/OraiswapV3.types.ts
[error] 185-185: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (3)
packages/contracts-sdk/src/OraiswapV3.client.ts (3)
14-14: New method added to the interfaceThe
incentivesFundManagermethod has been added to theOraiswapV3ReadOnlyInterface. This addition is consistent with the interface's structure and naming conventions.
209-209: Method binding in the constructorThe new
incentivesFundManagermethod is correctly bound to the class instance in the constructor, maintaining consistency with other methods.
9-9: Potential naming conflict with global BooleanThe static analysis tool flagged a potential issue with shadowing the global
Booleanproperty. However, this is a false positive in this context. TheBooleantype is correctly imported from the local types file and is not shadowing the globalBoolean.🧰 Tools
🪛 Biome
[error] 9-9: Do not shadow the global "Boolean" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
| } | { | ||
| incentives_fund_manager: {}; |
There was a problem hiding this comment.
Approve the new query, but refine the type definition.
The addition of the incentives_fund_manager query is a good complement to the new instantiation parameter. However, the use of {} as a type is not recommended.
Replace {} with a more specific type or void to improve type safety:
export type QueryMsg = {
// ... other queries ...
} | {
incentives_fund_manager: void;
} | {
// ... remaining queries ...
};This change addresses the static analysis warning and makes the type definition more explicit.
🧰 Tools
🪛 Biome
[error] 185-185: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (2)
packages/oraiswap-v3/tests/handler.spec.ts (1)
9-9: LGTM. Consider updating documentation.The addition of
deployIncentivesFundManagerto the imports is appropriate given the new functionality.Consider updating the documentation to reflect this new functionality if not already done.
packages/oraiswap-v3/src/helpers.ts (1)
570-571: Consider using precise decimal calculations forslippageMultiplierThe calculation of
slippageMultiplierinvolves multiplying and dividing by large numbers to adjust for slippage:const slippageMultiplier = BigInt(Math.floor((100 - slippage) * 1000)); message.minimumLiquidity = res.l ? (BigInt(res.l) * slippageMultiplier) / 100_000n : 0n;This approach can lead to precision loss and might not accurately reflect the intended slippage adjustment. Consider using a precise decimal or big number library to handle decimal calculations more accurately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
packages/oraiswap-v3/tests/data/oraiswap-v3.wasmis excluded by!**/*.wasmyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
- packages/oraiswap-v3/package.json (2 hunks)
- packages/oraiswap-v3/src/helpers.ts (3 hunks)
- packages/oraiswap-v3/tests/handler.spec.ts (4 hunks)
- packages/oraiswap-v3/tests/test-common.ts (2 hunks)
- packages/oraiswap-v3/tests/zap-consumer.spec.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/oraiswap-v3/package.json
🔇 Additional comments (9)
packages/oraiswap-v3/tests/handler.spec.ts (4)
190-192: LGTM. Improved formatting.The formatting adjustment in the
createIncentivemethod call improves code readability without changing functionality.
Line range hint
1-368: Overall LGTM. New component added to test setup.The changes in this file primarily involve the addition of an incentives fund manager component to the test setup and a minor update to the approve function call. While these changes don't alter the core functionality of the tests, they introduce a new element that could potentially affect test behavior.
To ensure the integrity of the test suite:
- Run the entire test suite and verify that all tests pass with the new setup.
- Review the coverage report to ensure that the new component is adequately tested.
- Consider adding specific tests for the new incentives fund manager functionality if not already present.
307-308: LGTM. Verify contract interface update.The addition of the
tokenIdparameter to theapprovemethod call is appropriate.Please verify that this change is consistent with any updates to the contract's approve function signature. Run the following script to check the contract interface:
#!/bin/bash # Description: Check the contract interface for the approve function # Test: Search for the approve function definition rg --type rust -A 5 'fn approve' # Note: Adjust the search pattern if necessary based on your contract structure
46-49: LGTM. Verify impact on existing tests.The addition of
fundManagerand its integration into the OraiswapV3Client instantiation is appropriate. This change introduces a new component to the test setup.Please verify that this change doesn't unintentionally affect the behavior of existing tests. Run the following script to check for any failing tests:
Also applies to: 58-58
packages/oraiswap-v3/tests/test-common.ts (2)
81-84: Verify that themulticallcontract requires no initialization parametersIn
deployMultiCall, an empty object{}is passed as the initialization message todeployContract. Ensure that themulticallcontract does not require any specific initialization parameters and will function correctly when instantiated with an empty message.If necessary, update the initialization message to include required parameters based on the contract's specification.
86-114: Consistent typing formsgparameters in deployment functionsThe deployment functions (
deployIncentivesFundManager,deployDexV3,deployOracle,deployFactory,deployMixedRouter,deployZapper) acceptmsgparameters with specific types, which is good for type safety. Ensure that these types align with the expected initialization messages for each contract.Consider applying similar type safety improvements to
deployContractas previously suggested.packages/oraiswap-v3/tests/zap-consumer.spec.ts (2)
74-85: Test assertions are properly validating token transfersThe test case for the
connectfunction correctly checks the balances before and after the transfer, ensuring that theconnectmethod assigns the signer properly and that the transfer functionality works as expected.
170-175: Verify initial sqrt prices and tick valuesThe initialization of pools with
initSqrtPriceandinitTickshould be carefully verified to ensure they correspond to the intended starting prices and tick positions.Would you like to confirm that the
initSqrtPriceandinitTickvalues match the expected initial prices for the pools?You can run the following script to calculate and compare the sqrt prices and ticks:
Also applies to: 181-186, 192-197
packages/oraiswap-v3/src/helpers.ts (1)
499-502: Unused propertyisSingleSideremains inbuildZapInMessageOptionsThe
isSingleSideproperty is still declared but not used anywhere in the code. Keeping unused properties can lead to confusion and increase maintenance overhead.
|
|
||
| export const senderAddress = "orai1g4h64yjt0fvzv5v2j8tyfnpe5kmnetejvfgs7g"; | ||
| export const bobAddress = "orai1602dkqjvh4s7ryajnz2uwhr8vetrwr8nekpxv5"; | ||
|
|
||
| export const deployer = "orai1swus8mwu8xjulawqxdwh8hvg4gknh2c64tuc0k"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using configuration or environment variables for the deployer address
Hardcoding the deployer address reduces flexibility and may cause issues if the address changes. Consider using a configuration file or environment variables to manage addresses, which will make the code more maintainable and adaptable to different environments.
| export const deployContract = async (name: string, initMsg: any) => { | ||
| const { codeId } = await client.upload( | ||
| deployer, | ||
| fs.readFileSync(path.resolve(__dirname, "data", `${name}.wasm`)), | ||
| "auto" | ||
| ); | ||
|
|
||
| const { contractAddress } = await client.instantiate( | ||
| deployer, | ||
| codeId, | ||
| initMsg, | ||
| name, | ||
| "auto" | ||
| ); | ||
|
|
||
| return contractAddress; | ||
| }; |
There was a problem hiding this comment.
Avoid using the any type for initMsg parameter
Using any for the initMsg parameter in deployContract bypasses TypeScript's type checking, potentially leading to runtime errors. Define a generic type parameter or a specific interface to enhance type safety and code reliability.
Apply this change to improve type safety:
-export const deployContract = async (name: string, initMsg: any) => {
+export const deployContract = async <T>(name: string, initMsg: T) => {This introduces a generic type T for the initMsg parameter, allowing for better type inference and validation.
📝 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.
| export const deployContract = async (name: string, initMsg: any) => { | |
| const { codeId } = await client.upload( | |
| deployer, | |
| fs.readFileSync(path.resolve(__dirname, "data", `${name}.wasm`)), | |
| "auto" | |
| ); | |
| const { contractAddress } = await client.instantiate( | |
| deployer, | |
| codeId, | |
| initMsg, | |
| name, | |
| "auto" | |
| ); | |
| return contractAddress; | |
| }; | |
| export const deployContract = async <T>(name: string, initMsg: T) => { | |
| const { codeId } = await client.upload( | |
| deployer, | |
| fs.readFileSync(path.resolve(__dirname, "data", `${name}.wasm`)), | |
| "auto" | |
| ); | |
| const { contractAddress } = await client.instantiate( | |
| deployer, | |
| codeId, | |
| initMsg, | |
| name, | |
| "auto" | |
| ); | |
| return contractAddress; | |
| }; |
Add error handling to deployContract function
If an error occurs during the client.upload or client.instantiate calls, it will currently go unhandled, which might make debugging difficult. Wrap these calls in a try-catch block to handle exceptions gracefully and provide informative error messages.
Apply this change to incorporate error handling:
export const deployContract = async <T>(name: string, initMsg: T) => {
+ try {
const { codeId } = await client.upload(
deployer,
fs.readFileSync(path.resolve(__dirname, "data", `${name}.wasm`)),
"auto"
);
const { contractAddress } = await client.instantiate(
deployer,
codeId,
initMsg,
name,
"auto"
);
return contractAddress;
+ } catch (error) {
+ console.error(`Failed to deploy contract ${name}:`, error);
+ throw error;
+ }
};📝 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.
| export const deployContract = async (name: string, initMsg: any) => { | |
| const { codeId } = await client.upload( | |
| deployer, | |
| fs.readFileSync(path.resolve(__dirname, "data", `${name}.wasm`)), | |
| "auto" | |
| ); | |
| const { contractAddress } = await client.instantiate( | |
| deployer, | |
| codeId, | |
| initMsg, | |
| name, | |
| "auto" | |
| ); | |
| return contractAddress; | |
| }; | |
| export const deployContract = async <T>(name: string, initMsg: T) => { | |
| try { | |
| const { codeId } = await client.upload( | |
| deployer, | |
| fs.readFileSync(path.resolve(__dirname, "data", `${name}.wasm`)), | |
| "auto" | |
| ); | |
| const { contractAddress } = await client.instantiate( | |
| deployer, | |
| codeId, | |
| initMsg, | |
| name, | |
| "auto" | |
| ); | |
| return contractAddress; | |
| } catch (error) { | |
| console.error(`Failed to deploy contract ${name}:`, error); | |
| throw error; | |
| } | |
| }; |
| // move signer to dynamic signing of an object | ||
| declare module "@oraichain/oraidex-contracts-sdk" { | ||
| interface OraiswapTokenClient { | ||
| connect(signer: string): OraiswapTokenClient; | ||
| } | ||
| } | ||
| OraiswapTokenClient.prototype.connect = function (signer: string): OraiswapTokenClient { | ||
| this.sender = signer; | ||
| return this; | ||
| }; |
There was a problem hiding this comment.
Avoid modifying the prototypes of external modules
Modifying the prototype of OraiswapTokenClient can lead to unintended side effects throughout the codebase, especially if other parts of the application rely on the original behavior of the client. This practice can make the code harder to maintain and debug.
Consider extending the class or using a wrapper function to add the connect method without altering the original prototype.
Refactor Suggestion:
Create a subclass to extend OraiswapTokenClient:
class ExtendedOraiswapTokenClient extends OraiswapTokenClient {
connect(signer: string): OraiswapTokenClient {
this.sender = signer;
return this;
}
}Use the extended class in your tests:
tokenTest = new ExtendedOraiswapTokenClient(client, tokenAddress);| declare module "@oraichain/oraidex-contracts-sdk" { | ||
| interface OraiswapV3Client { | ||
| connect(signer: string): OraiswapV3Client; | ||
| } | ||
| } | ||
| OraiswapV3Client.prototype.connect = function (signer: string): OraiswapV3Client { | ||
| this.sender = signer; | ||
| return this; | ||
| }; |
There was a problem hiding this comment.
Avoid modifying the prototypes of external modules
Similarly, modifying the prototype of OraiswapV3Client can introduce unexpected behavior and complicate maintenance. It's better to extend the class or create a wrapper to add new functionalities like the connect method.
Refactor Suggestion:
Create a subclass to extend OraiswapV3Client:
class ExtendedOraiswapV3Client extends OraiswapV3Client {
connect(signer: string): OraiswapV3Client {
this.sender = signer;
return this;
}
}Use the extended class where needed:
oraiswapV3 = new ExtendedOraiswapV3Client(client, contractAddress);| protocol_fee: 0.25 * 10 ** 12, | ||
| incentives_fund_manager: incentivesFundManager.contractAddress | ||
| }); |
There was a problem hiding this comment.
Use integer arithmetic to prevent floating-point precision errors
Using floating-point numbers in calculations like 0.25 * 10 ** 12 can lead to precision issues due to how floating-point arithmetic works in JavaScript/TypeScript. Instead, use integer arithmetic to maintain precision, especially when dealing with financial values.
Apply this diff to use integer values:
- protocol_fee: 0.25 * 10 ** 12,
+ protocol_fee: 250_000_000_000, // Equivalent to 0.25 * 10^12
...
- fee: 0.003 * 10 ** 12,
+ fee: 3_000_000_000, // Equivalent to 0.003 * 10^12
...
- fee: 0.0005 * 10 ** 12,
+ fee: 500_000_000, // Equivalent to 0.0005 * 10^12Also applies to: 151-157, 173-175, 184-185, 195-196
| it("InRangeNoRouteThroughSelf", async () => { | ||
| // zap to pool usdt usdc, use oraix | ||
| const pools = await handler.getPools(); | ||
|
|
||
| await oraix.connect(deployer).mint({ | ||
| recipient: alice, | ||
| amount: (1000n * 10n ** 6n).toString() | ||
| }); | ||
| await usdc.connect(deployer).mint({ | ||
| recipient: alice, | ||
| amount: (1000n * 10n ** 6n).toString() | ||
| }); | ||
| await usdt.connect(deployer).mint({ | ||
| recipient: alice, | ||
| amount: (1000n * 10n ** 6n).toString() | ||
| }); | ||
|
|
||
| await oraix.connect(alice).increaseAllowance({ | ||
| spender: oraiswapV3.contractAddress, | ||
| amount: (1000n * 10n ** 6n).toString() | ||
| }); | ||
| await usdc.connect(alice).increaseAllowance({ | ||
| spender: oraiswapV3.contractAddress, | ||
| amount: (1000n * 10n ** 6n).toString() | ||
| }); | ||
| await usdt.connect(alice).increaseAllowance({ | ||
| spender: oraiswapV3.contractAddress, | ||
| amount: (1000n * 10n ** 6n).toString() | ||
| }); | ||
|
|
||
| const oraixUsdtPK = newPoolKey(oraix.contractAddress, usdt.contractAddress, feeTier1); // oraix < usdt | ||
| const usdcOraixPK = newPoolKey(oraix.contractAddress, usdc.contractAddress, feeTier1); // usdc < oraix | ||
| const usdcUsdtPK = newPoolKey(usdt.contractAddress, usdc.contractAddress, feeTier2); // usdc < usdt | ||
|
|
||
| await oraiswapV3.connect(alice).createPosition({ | ||
| poolKey: oraixUsdtPK, | ||
| lowerTick: 13700, | ||
| upperTick: 13900, | ||
| liquidityDelta: (10000n * 10n ** 6n).toString(), | ||
| slippageLimitLower: getGlobalMinSqrtPrice().toString(), | ||
| slippageLimitUpper: getGlobalMaxSqrtPrice().toString() | ||
| }); | ||
| await oraiswapV3.connect(alice).createPosition({ | ||
| poolKey: usdcOraixPK, | ||
| lowerTick: -14000, | ||
| upperTick: -13800, | ||
| liquidityDelta: (10000n * 10n ** 6n).toString(), | ||
| slippageLimitLower: getGlobalMinSqrtPrice().toString(), | ||
| slippageLimitUpper: getGlobalMaxSqrtPrice().toString() | ||
| }); | ||
| await oraiswapV3.connect(alice).createPosition({ | ||
| poolKey: usdcUsdtPK, | ||
| lowerTick: -10, | ||
| upperTick: 10, | ||
| liquidityDelta: (10000n * 10n ** 6n).toString(), | ||
| slippageLimitLower: getGlobalMinSqrtPrice().toString(), | ||
| slippageLimitUpper: getGlobalMaxSqrtPrice().toString() | ||
| }); | ||
|
|
||
| const oraixUsdt = await handler.getPool(oraixUsdtPK); | ||
| const usdcOraix = await handler.getPool(usdcOraixPK); | ||
| const usdcUsdt = await handler.getPool(usdcUsdtPK); | ||
|
|
||
| // console.log({ oraixUsdt, usdcOraix, usdcUsdt }); | ||
|
|
||
| const positions = await handler.allPositions(); | ||
| const liquidityOraixUsdt = await handler.getPairLiquidityValues(oraixUsdt, positions); | ||
| const liquidityUsdcOraix = await handler.getPairLiquidityValues(usdcOraix, positions); | ||
| const liquidityUsdcUsdt = await handler.getPairLiquidityValues(usdcUsdt, positions); | ||
| }); |
There was a problem hiding this comment.
Unused variables and commented-out code
Within the InRangeNoRouteThroughSelf test case, there are variables assigned that are not utilized, and there's a commented-out console.log statement. Unused variables can clutter the code and make it harder to read.
Consider removing or utilizing the unused variables:
- const pools = await handler.getPools();
- // console.log({ oraixUsdt, usdcOraix, usdcUsdt });
- const positions = await handler.allPositions();
- const liquidityOraixUsdt = await handler.getPairLiquidityValues(oraixUsdt, positions);
- const liquidityUsdcOraix = await handler.getPairLiquidityValues(usdcOraix, positions);
- const liquidityUsdcUsdt = await handler.getPairLiquidityValues(usdcUsdt, positions);If these variables are intended for future use or for assertion purposes, ensure they are integrated into the test case accordingly.
📝 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.
| it("InRangeNoRouteThroughSelf", async () => { | |
| // zap to pool usdt usdc, use oraix | |
| const pools = await handler.getPools(); | |
| await oraix.connect(deployer).mint({ | |
| recipient: alice, | |
| amount: (1000n * 10n ** 6n).toString() | |
| }); | |
| await usdc.connect(deployer).mint({ | |
| recipient: alice, | |
| amount: (1000n * 10n ** 6n).toString() | |
| }); | |
| await usdt.connect(deployer).mint({ | |
| recipient: alice, | |
| amount: (1000n * 10n ** 6n).toString() | |
| }); | |
| await oraix.connect(alice).increaseAllowance({ | |
| spender: oraiswapV3.contractAddress, | |
| amount: (1000n * 10n ** 6n).toString() | |
| }); | |
| await usdc.connect(alice).increaseAllowance({ | |
| spender: oraiswapV3.contractAddress, | |
| amount: (1000n * 10n ** 6n).toString() | |
| }); | |
| await usdt.connect(alice).increaseAllowance({ | |
| spender: oraiswapV3.contractAddress, | |
| amount: (1000n * 10n ** 6n).toString() | |
| }); | |
| const oraixUsdtPK = newPoolKey(oraix.contractAddress, usdt.contractAddress, feeTier1); // oraix < usdt | |
| const usdcOraixPK = newPoolKey(oraix.contractAddress, usdc.contractAddress, feeTier1); // usdc < oraix | |
| const usdcUsdtPK = newPoolKey(usdt.contractAddress, usdc.contractAddress, feeTier2); // usdc < usdt | |
| await oraiswapV3.connect(alice).createPosition({ | |
| poolKey: oraixUsdtPK, | |
| lowerTick: 13700, | |
| upperTick: 13900, | |
| liquidityDelta: (10000n * 10n ** 6n).toString(), | |
| slippageLimitLower: getGlobalMinSqrtPrice().toString(), | |
| slippageLimitUpper: getGlobalMaxSqrtPrice().toString() | |
| }); | |
| await oraiswapV3.connect(alice).createPosition({ | |
| poolKey: usdcOraixPK, | |
| lowerTick: -14000, | |
| upperTick: -13800, | |
| liquidityDelta: (10000n * 10n ** 6n).toString(), | |
| slippageLimitLower: getGlobalMinSqrtPrice().toString(), | |
| slippageLimitUpper: getGlobalMaxSqrtPrice().toString() | |
| }); | |
| await oraiswapV3.connect(alice).createPosition({ | |
| poolKey: usdcUsdtPK, | |
| lowerTick: -10, | |
| upperTick: 10, | |
| liquidityDelta: (10000n * 10n ** 6n).toString(), | |
| slippageLimitLower: getGlobalMinSqrtPrice().toString(), | |
| slippageLimitUpper: getGlobalMaxSqrtPrice().toString() | |
| }); | |
| const oraixUsdt = await handler.getPool(oraixUsdtPK); | |
| const usdcOraix = await handler.getPool(usdcOraixPK); | |
| const usdcUsdt = await handler.getPool(usdcUsdtPK); | |
| // console.log({ oraixUsdt, usdcOraix, usdcUsdt }); | |
| const positions = await handler.allPositions(); | |
| const liquidityOraixUsdt = await handler.getPairLiquidityValues(oraixUsdt, positions); | |
| const liquidityUsdcOraix = await handler.getPairLiquidityValues(usdcOraix, positions); | |
| const liquidityUsdcUsdt = await handler.getPairLiquidityValues(usdcUsdt, positions); | |
| }); | |
| it("InRangeNoRouteThroughSelf", async () => { | |
| // zap to pool usdt usdc, use oraix | |
| await oraix.connect(deployer).mint({ | |
| recipient: alice, | |
| amount: (1000n * 10n ** 6n).toString() | |
| }); | |
| await usdc.connect(deployer).mint({ | |
| recipient: alice, | |
| amount: (1000n * 10n ** 6n).toString() | |
| }); | |
| await usdt.connect(deployer).mint({ | |
| recipient: alice, | |
| amount: (1000n * 10n ** 6n).toString() | |
| }); | |
| await oraix.connect(alice).increaseAllowance({ | |
| spender: oraiswapV3.contractAddress, | |
| amount: (1000n * 10n ** 6n).toString() | |
| }); | |
| await usdc.connect(alice).increaseAllowance({ | |
| spender: oraiswapV3.contractAddress, | |
| amount: (1000n * 10n ** 6n).toString() | |
| }); | |
| await usdt.connect(alice).increaseAllowance({ | |
| spender: oraiswapV3.contractAddress, | |
| amount: (1000n * 10n ** 6n).toString() | |
| }); | |
| const oraixUsdtPK = newPoolKey(oraix.contractAddress, usdt.contractAddress, feeTier1); // oraix < usdt | |
| const usdcOraixPK = newPoolKey(oraix.contractAddress, usdc.contractAddress, feeTier1); // usdc < oraix | |
| const usdcUsdtPK = newPoolKey(usdt.contractAddress, usdc.contractAddress, feeTier2); // usdc < usdt | |
| await oraiswapV3.connect(alice).createPosition({ | |
| poolKey: oraixUsdtPK, | |
| lowerTick: 13700, | |
| upperTick: 13900, | |
| liquidityDelta: (10000n * 10n ** 6n).toString(), | |
| slippageLimitLower: getGlobalMinSqrtPrice().toString(), | |
| slippageLimitUpper: getGlobalMaxSqrtPrice().toString() | |
| }); | |
| await oraiswapV3.connect(alice).createPosition({ | |
| poolKey: usdcOraixPK, | |
| lowerTick: -14000, | |
| upperTick: -13800, | |
| liquidityDelta: (10000n * 10n ** 6n).toString(), | |
| slippageLimitLower: getGlobalMinSqrtPrice().toString(), | |
| slippageLimitUpper: getGlobalMaxSqrtPrice().toString() | |
| }); | |
| await oraiswapV3.connect(alice).createPosition({ | |
| poolKey: usdcUsdtPK, | |
| lowerTick: -10, | |
| upperTick: 10, | |
| liquidityDelta: (10000n * 10n ** 6n).toString(), | |
| slippageLimitLower: getGlobalMinSqrtPrice().toString(), | |
| slippageLimitUpper: getGlobalMaxSqrtPrice().toString() | |
| }); | |
| const oraixUsdt = await handler.getPool(oraixUsdtPK); | |
| const usdcOraix = await handler.getPool(usdcOraixPK); | |
| const usdcUsdt = await handler.getPool(usdcUsdtPK); | |
| }); |
| if (buildZapInMessageOptions) { | ||
| if (buildZapInMessageOptions.isTokenX) { | ||
| message.amountY = "0"; | ||
| } else { | ||
| message.amountX = "0"; | ||
| } | ||
| message.routes = generateMessageSwapOperation([actualAmountXReceived], slippage); | ||
| } else { |
There was a problem hiding this comment.
Ensure isTokenX is always defined when using buildZapInMessageOptions
In the populateMessageZapIn function, when buildZapInMessageOptions is provided, isTokenX is accessed without verifying its existence. This could lead to runtime errors if isTokenX is undefined.
Consider adding a type check to ensure isTokenX is defined:
if (buildZapInMessageOptions) {
+ if (typeof buildZapInMessageOptions.isTokenX !== 'boolean') {
+ throw new Error('buildZapInMessageOptions.isTokenX must be a boolean');
+ }
if (buildZapInMessageOptions.isTokenX) {
message.amountY = "0";
} else {
message.amountX = "0";
}
message.routes = generateMessageSwapOperation([actualAmountXReceived], slippage);
} else {
message.routes = generateMessageSwapOperation([actualAmountXReceived, actualAmountYReceived], slippage);
}This ensures robustness by validating the expected type of isTokenX.
📝 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.
| if (buildZapInMessageOptions) { | |
| if (buildZapInMessageOptions.isTokenX) { | |
| message.amountY = "0"; | |
| } else { | |
| message.amountX = "0"; | |
| } | |
| message.routes = generateMessageSwapOperation([actualAmountXReceived], slippage); | |
| } else { | |
| if (buildZapInMessageOptions) { | |
| if (typeof buildZapInMessageOptions.isTokenX !== 'boolean') { | |
| throw new Error('buildZapInMessageOptions.isTokenX must be a boolean'); | |
| } | |
| if (buildZapInMessageOptions.isTokenX) { | |
| message.amountY = "0"; | |
| } else { | |
| message.amountX = "0"; | |
| } | |
| message.routes = generateMessageSwapOperation([actualAmountXReceived], slippage); | |
| } else { |
| if (buildZapInMessageOptions) { | ||
| const res = buildZapInMessageOptions.isTokenX | ||
| ? getLiquidityByX(BigInt(actualAmountXReceived.returnAmount), lowerTick, upperTick, sqrtPrice, true) | ||
| : getLiquidityByY(BigInt(actualAmountYReceived.returnAmount), lowerTick, upperTick, sqrtPrice, true); | ||
| const slippageMultiplier = BigInt(Math.floor((100 - slippage) * 1000)); | ||
| message.minimumLiquidity = res.l ? (BigInt(res.l) * slippageMultiplier) / 100_000n : 0n; | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Add type check for isTokenX in calculateMinimumLiquidity
In the calculateMinimumLiquidity function, when buildZapInMessageOptions is provided, isTokenX is accessed without verification. To prevent potential runtime errors, ensure that isTokenX is defined:
if (buildZapInMessageOptions) {
+ if (typeof buildZapInMessageOptions.isTokenX !== 'boolean') {
+ throw new Error('buildZapInMessageOptions.isTokenX must be a boolean');
+ }
const res = buildZapInMessageOptions.isTokenX
? getLiquidityByX(BigInt(actualAmountXReceived.returnAmount), lowerTick, upperTick, sqrtPrice, true)
: getLiquidityByY(BigInt(actualAmountYReceived.returnAmount), lowerTick, upperTick, sqrtPrice, true);Adding this check ensures that isTokenX has the expected type before use.
📝 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.
| if (buildZapInMessageOptions) { | |
| const res = buildZapInMessageOptions.isTokenX | |
| ? getLiquidityByX(BigInt(actualAmountXReceived.returnAmount), lowerTick, upperTick, sqrtPrice, true) | |
| : getLiquidityByY(BigInt(actualAmountYReceived.returnAmount), lowerTick, upperTick, sqrtPrice, true); | |
| const slippageMultiplier = BigInt(Math.floor((100 - slippage) * 1000)); | |
| message.minimumLiquidity = res.l ? (BigInt(res.l) * slippageMultiplier) / 100_000n : 0n; | |
| return; | |
| } | |
| if (buildZapInMessageOptions) { | |
| if (typeof buildZapInMessageOptions.isTokenX !== 'boolean') { | |
| throw new Error('buildZapInMessageOptions.isTokenX must be a boolean'); | |
| } | |
| const res = buildZapInMessageOptions.isTokenX | |
| ? getLiquidityByX(BigInt(actualAmountXReceived.returnAmount), lowerTick, upperTick, sqrtPrice, true) | |
| : getLiquidityByY(BigInt(actualAmountYReceived.returnAmount), lowerTick, upperTick, sqrtPrice, true); | |
| const slippageMultiplier = BigInt(Math.floor((100 - slippage) * 1000)); | |
| message.minimumLiquidity = res.l ? (BigInt(res.l) * slippageMultiplier) / 100_000n : 0n; | |
| return; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
packages/oraiswap-v3/src/main.ts (1)
32-32: Simplify the Fee Tier Calculation in Pool StringConsider simplifying the calculation
(0.3 / 100) * 10 ** 12to3e9for improved readability and to avoid unnecessary computations at runtime.Apply this diff to simplify the calculation:
- const pool = `${OSMOSIS_ORAICHAIN_DENOM}-orai-${(0.3 / 100) * 10 ** 12}-100`; + const pool = `${OSMOSIS_ORAICHAIN_DENOM}-orai-${3e9}-100`;packages/oraiswap-v3/src/helpers.ts (1)
Line range hint
499-573: Add unit tests for the newbuildZapInMessageOptionsfunctionalityThe introduction of
buildZapInMessageOptionsadds conditional logic paths that should be tested to ensure correctness and prevent future regressions. Consider adding unit tests that cover scenarios with and withoutbuildZapInMessageOptions, and with different values ofisTokenX.Would you like assistance in creating unit tests for these new code paths or opening a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/oraiswap-v3/package.json (2 hunks)
- packages/oraiswap-v3/src/helpers.ts (2 hunks)
- packages/oraiswap-v3/src/main.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/oraiswap-v3/package.json
🔇 Additional comments (7)
packages/oraiswap-v3/src/main.ts (6)
3-3: Addition of 'KWT_CONTRACT' ImportThe import of
KWT_CONTRACTis appropriate and correctly added to the import statement.
7-8: Importing 'OSMO' and 'OSMOSIS_ORAICHAIN_DENOM'The additions of
OSMOandOSMOSIS_ORAICHAIN_DENOMto the imports are correct and necessary for the code.
14-15: Importing Helper Functions and ModulesThe imports of
extractAddress,parsePoolKey, andgetTickAtSqrtPriceare appropriately added. These functions are essential for parsing pool keys and handling price calculations.
26-26: Setting 'protocols' to ['OraidexV3']The
protocolsconfiguration insmartRouteConfig.swapOptionsis set to["OraidexV3"], which specifies the use of the Oraidex V3 protocol. This is correctly implemented.
31-31: Finding 'tokenIn' by NameUsing
oraichainTokens.find((t) => t.name === "USDT")to locate thetokenInby its name enhances code readability and clarity.
40-41: Retrieving Tick Spacing and Current Tick IndexThe retrieval of
tickSpacingandcurrentTickfrompoolKeyand the pool data is correctly implemented, ensuring accurate tick calculations for liquidity operations.packages/oraiswap-v3/src/helpers.ts (1)
499-502: Remove unused propertyisSingleSidefrombuildZapInMessageOptionsThe
isSingleSideproperty is declared but not used anywhere in the code. Keeping unused properties can lead to confusion and increase maintenance overhead.
| const res = await zapper.processZapOutPositionLiquidity({ | ||
| owner: "orai1hvr9d72r5um9lvt0rpkd4r75vrsqtw6yujhqs2", | ||
| tokenId: 4275, | ||
| tokenOut: tokenIn, | ||
| zapFee: 0, |
There was a problem hiding this comment.
Avoid Hardcoding 'owner' Address and 'tokenId'
Hardcoding the owner address and tokenId may lead to security risks and reduce code flexibility. It's advisable to parameterize these values or retrieve them from a secure configuration or user input.
Consider modifying the code as follows to pass these values dynamically:
const res = await zapper.processZapOutPositionLiquidity({
- owner: "orai1hvr9d72r5um9lvt0rpkd4r75vrsqtw6yujhqs2",
- tokenId: 4275,
+ owner: OWNER_ADDRESS, // Replace with dynamic value
+ tokenId: TOKEN_ID, // Replace with dynamic value
tokenOut: tokenIn,
zapFee: 0,
});This change enhances maintainability and security by avoiding hardcoded sensitive information.
Committable suggestion was skipped due to low confidence.
| if (buildZapInMessageOptions) { | ||
| const res = buildZapInMessageOptions.isTokenX | ||
| ? getLiquidityByX(BigInt(actualAmountXReceived.returnAmount), lowerTick, upperTick, sqrtPrice, true) | ||
| : getLiquidityByY(BigInt(actualAmountYReceived.returnAmount), lowerTick, upperTick, sqrtPrice, true); | ||
| const slippageMultiplier = BigInt(Math.floor((100 - slippage) * 1000)); | ||
| message.minimumLiquidity = res.l ? (BigInt(res.l) * slippageMultiplier) / 100_000n : 0n; | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle slippage calculations with precise decimal arithmetic
The current implementation multiplies the slippage by 1000 and uses integer arithmetic to calculate slippageMultiplier. This may lead to precision loss for small slippage values. Consider using a decimal library like BigNumber.js to handle precise decimal computations, ensuring accurate minimum liquidity calculations.
Apply this refactor to improve precision:
-const slippageMultiplier = BigInt(Math.floor((100 - slippage) * 1000));
-message.minimumLiquidity = res.l ? (BigInt(res.l) * slippageMultiplier) / 100_000n : 0n;
+const slippageDecimal = new BigDecimal(100 - slippage).div(100);
+message.minimumLiquidity = res.l ? BigInt(new BigDecimal(res.l.toString()).mul(slippageDecimal).toFixed(0)) : 0n;This change ensures that the slippage is accurately applied without losing precision due to integer division.
Committable suggestion was skipped due to low confidence.
| buildZapInMessageOptions?: buildZapInMessageOptions | ||
| ) => { | ||
| message.assetIn = parseAsset(tokenIn, amountIn); | ||
| message.amountToX = amountInToX.toString(); | ||
| message.amountToY = amountInToY.toString(); | ||
| message.amountX = actualAmountXReceived.returnAmount; | ||
| message.amountY = actualAmountYReceived.returnAmount; | ||
| message.poolKey = poolKey; | ||
| message.sqrtPrice = BigInt(pool.sqrt_price); | ||
| message.tickLowerIndex = lowerTick; | ||
| message.tickUpperIndex = upperTick; | ||
| message.routes = generateMessageSwapOperation([actualAmountXReceived, actualAmountYReceived], slippage); | ||
| message.currentSqrtPrice = sqrtPrice; | ||
|
|
||
| if (buildZapInMessageOptions) { | ||
| if (buildZapInMessageOptions.isTokenX) { | ||
| message.amountY = "0"; | ||
| } else { | ||
| message.amountX = "0"; | ||
| } | ||
| message.routes = generateMessageSwapOperation([actualAmountXReceived], slippage); | ||
| } else { | ||
| message.routes = generateMessageSwapOperation([actualAmountXReceived, actualAmountYReceived], slippage); | ||
| } | ||
|
|
||
| calculateSwapFee(message); | ||
|
|
||
| calculateMinimumLiquidity( | ||
| message, | ||
| actualAmountXReceived, | ||
| actualAmountYReceived, | ||
| lowerTick, | ||
| upperTick, | ||
| sqrtPrice, | ||
| slippage, | ||
| buildZapInMessageOptions | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring populateMessageZapIn to improve readability
The populateMessageZapIn function now has a large number of parameters, which can make it harder to read and maintain. Consider refactoring the function to accept an options object or to group related parameters into objects.
Apply this refactor to simplify the function signature:
-export const populateMessageZapIn = (
- message: ZapInLiquidityResponse,
- actualAmountXReceived: SmartRouteResponse,
- actualAmountYReceived: SmartRouteResponse,
- sqrtPrice: bigint,
- poolKey: PoolKey,
- lowerTick: number,
- upperTick: number,
- slippage: number,
- buildZapInMessageOptions?: buildZapInMessageOptions
-) => {
+interface PopulateMessageZapInOptions {
+ message: ZapInLiquidityResponse;
+ actualAmountXReceived: SmartRouteResponse;
+ actualAmountYReceived: SmartRouteResponse;
+ sqrtPrice: bigint;
+ poolKey: PoolKey;
+ lowerTick: number;
+ upperTick: number;
+ slippage: number;
+ buildZapInMessageOptions?: buildZapInMessageOptions;
+}
+
+export const populateMessageZapIn = (options: PopulateMessageZapInOptions) => {
+ const {
+ message,
+ actualAmountXReceived,
+ actualAmountYReceived,
+ sqrtPrice,
+ poolKey,
+ lowerTick,
+ upperTick,
+ slippage,
+ buildZapInMessageOptions
+ } = options;This refactor enhances readability and makes it easier to manage parameters.
📝 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.
| buildZapInMessageOptions?: buildZapInMessageOptions | |
| ) => { | |
| message.assetIn = parseAsset(tokenIn, amountIn); | |
| message.amountToX = amountInToX.toString(); | |
| message.amountToY = amountInToY.toString(); | |
| message.amountX = actualAmountXReceived.returnAmount; | |
| message.amountY = actualAmountYReceived.returnAmount; | |
| message.poolKey = poolKey; | |
| message.sqrtPrice = BigInt(pool.sqrt_price); | |
| message.tickLowerIndex = lowerTick; | |
| message.tickUpperIndex = upperTick; | |
| message.routes = generateMessageSwapOperation([actualAmountXReceived, actualAmountYReceived], slippage); | |
| message.currentSqrtPrice = sqrtPrice; | |
| if (buildZapInMessageOptions) { | |
| if (buildZapInMessageOptions.isTokenX) { | |
| message.amountY = "0"; | |
| } else { | |
| message.amountX = "0"; | |
| } | |
| message.routes = generateMessageSwapOperation([actualAmountXReceived], slippage); | |
| } else { | |
| message.routes = generateMessageSwapOperation([actualAmountXReceived, actualAmountYReceived], slippage); | |
| } | |
| calculateSwapFee(message); | |
| calculateMinimumLiquidity( | |
| message, | |
| actualAmountXReceived, | |
| actualAmountYReceived, | |
| lowerTick, | |
| upperTick, | |
| sqrtPrice, | |
| slippage, | |
| buildZapInMessageOptions | |
| ); | |
| interface PopulateMessageZapInOptions { | |
| message: ZapInLiquidityResponse; | |
| actualAmountXReceived: SmartRouteResponse; | |
| actualAmountYReceived: SmartRouteResponse; | |
| sqrtPrice: bigint; | |
| poolKey: PoolKey; | |
| lowerTick: number; | |
| upperTick: number; | |
| slippage: number; | |
| buildZapInMessageOptions?: buildZapInMessageOptions; | |
| } | |
| export const populateMessageZapIn = (options: PopulateMessageZapInOptions) => { | |
| const { | |
| message, | |
| actualAmountXReceived, | |
| actualAmountYReceived, | |
| sqrtPrice, | |
| poolKey, | |
| lowerTick, | |
| upperTick, | |
| slippage, | |
| buildZapInMessageOptions | |
| } = options; | |
| message.amountX = actualAmountXReceived.returnAmount; | |
| message.amountY = actualAmountYReceived.returnAmount; | |
| message.poolKey = poolKey; | |
| message.tickLowerIndex = lowerTick; | |
| message.tickUpperIndex = upperTick; | |
| message.currentSqrtPrice = sqrtPrice; | |
| if (buildZapInMessageOptions) { | |
| if (buildZapInMessageOptions.isTokenX) { | |
| message.amountY = "0"; | |
| } else { | |
| message.amountX = "0"; | |
| } | |
| message.routes = generateMessageSwapOperation([actualAmountXReceived], slippage); | |
| } else { | |
| message.routes = generateMessageSwapOperation([actualAmountXReceived, actualAmountYReceived], slippage); | |
| } | |
| calculateSwapFee(message); | |
| calculateMinimumLiquidity( | |
| message, | |
| actualAmountXReceived, | |
| actualAmountYReceived, | |
| lowerTick, | |
| upperTick, | |
| sqrtPrice, | |
| slippage, | |
| buildZapInMessageOptions | |
| ); |
Summary by CodeRabbit
New Features
@oraichain/oraiswap-v3package to "0.1.20-beta.2."@oraichain/oraidex-commonpackage to "1.1.21."@oraichain/oraidex-contracts-sdkpackage to "1.0.53-beta.2."Bug Fixes