fix: use normalized decimal count for fromAmount calculation #164
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes primarily involve modifications to the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
test/unit/util.test.ts (2)
21-28: Consider simplifying mock cleanup.Using both
restoreAllMocksandclearAllMocksis redundant here.restoreAllMocksalready resets the mock's implementation, whileclearAllMocksonly clears the mock's history. You can safely removeclearAllMocks.afterEach(() => { jest.restoreAllMocks() - jest.clearAllMocks() })
29-69: Consider adding more test cases for comprehensive coverage.While the current test case validates the happy path with different decimal places (which aligns with the PR's objective), consider adding the following test cases:
- Error cases:
- Invalid token addresses
- API failures
- Invalid decimal values
- Edge cases:
- Same number of decimals for both tokens
- Maximum possible decimal difference
- Zero amount handling
- Slippage calculation verification:
- Verify that the returned amount includes the correct slippage adjustment
- API request validation:
- Verify the correct URLs are being called for price fetching
Here's an example of an additional test case for API validation:
it('should call API with correct URLs', async () => { // ... setup code ... await prepareXchainFromAmountCalculation({ fromToken, toToken, toAmount, slippagePercentage, }) expect(mockFetch).toHaveBeenNthCalledWith( 1, expect.stringContaining(`tokenAddress=${fromToken.address}`) ) expect(mockFetch).toHaveBeenNthCalledWith( 2, expect.stringContaining(`tokenAddress=${toToken.address}`) ) })src/util.ts (1)
745-745: Consider adding test cases for decimal normalization.To ensure the decimal normalization remains correct, consider adding test cases that cover:
- Source token with more decimals than target token
- Source token with fewer decimals than target token
- Tokens with equal decimals
Would you like me to help generate these test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/util.ts(1 hunks)test/unit/util.test.ts(2 hunks)
🔇 Additional comments (3)
test/unit/util.test.ts (2)
1-3: LGTM! Clean import setup with proper mocking.
The import statements are clear and the mock setup follows Jest best practices.
60-65: Verify integration with peanut-ui repository.
Since this change will be used in the peanut-ui repository (PR #508), consider adding integration tests that simulate the exact usage patterns from the UI to ensure compatibility.
src/util.ts (1)
745-745: LGTM! The decimal normalization fix looks correct.
The change to use fromToken.decimals instead of normalizedDecimalCount in the final formatting ensures that the returned amount matches the decimal precision of the source token, which is the expected behavior for cross-chain token swaps.
Let's verify the decimal normalization with different token combinations:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/util.ts (1)
768-768: Remove unused variable from destructuring.The
_variable is assigned but never used. Consider removing it from the destructuring assignment.-const [_, intPart, decimals] = match +const [, intPart, decimals] = match🧰 Tools
🪛 eslint
[error] 768-768: '_' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/util.ts(2 hunks)test/unit/util.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/unit/util.test.ts
🧰 Additional context used
🪛 eslint
src/util.ts
[error] 768-768: '_' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (2)
src/util.ts (2)
745-746: LGTM! Decimal normalization looks correct.
The changes properly handle decimal precision by first formatting with the normalized decimal count and then adjusting to match the source token's decimals.
764-771: LGTM! Well-implemented string formatting utility.
The stringToFixed function:
- Correctly handles decimal string formatting
- Properly manages edge cases (no decimal point, empty decimal part)
- Uses appropriate padding and slicing for precision control
🧰 Tools
🪛 eslint
[error] 768-768: '_' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
When the fromToken has fewer decimals than toToken, the amount returned by prepareXchainFromAmountCalculation was wrong because of mismatch in the decimals used for slippage calculation and the final decimals used to format the amount.
Afther packaging, get package in peanut-ui: peanutprotocol/peanut-ui#508