Conversation
WalkthroughThe condition in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant JS_API
participant Vault
User->>JS_API: getVaultApprovalCalldata(rpc_url, vault, deposit_amount)
JS_API->>Vault: read_allowance(owner, transaction_args)
Vault-->>JS_API: allowance
alt allowance >= deposit_amount
JS_API-->>User: Error (InvalidAmount)
else
JS_API->>Vault: get_approve_calldata(transaction_args)
Vault-->>JS_API: calldata
JS_API-->>User: VaultCalldataResult(calldata)
end
Assessment against linked issues
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
crates/js_api/src/subgraph/vault.rs(2 hunks)packages/orderbook/test/js_api/vault.test.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/orderbook/test/js_api/vault.test.ts
[error] 324-324: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 340-340: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
🔇 Additional comments (3)
crates/js_api/src/subgraph/vault.rs (2)
143-143: LGTM! Correct fix for the allowance check logic.The change from
allowance > deposit_amounttoallowance >= deposit_amountproperly handles the edge case where the allowance exactly equals the deposit amount. When the allowance is sufficient (greater than or equal to the deposit amount), no approval calldata should be generated.
576-579: Good test coverage for the edge case.The new test case properly verifies that when the allowance equals the deposit amount (both 100), the function correctly returns an
InvalidAmounterror, ensuring no unnecessary approval calldata is generated.packages/orderbook/test/js_api/vault.test.ts (1)
313-327: Well-designed test case for equal allowance scenario.This test properly verifies that when the requested approval amount equals the current allowance (both 100), the function correctly rejects the request with an "Invalid amount" error, consistent with the updated logic in the Rust implementation.
🧰 Tools
🪛 Biome (1.9.4)
[error] 324-324: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
| it('should handle approval amount less than allowance', async () => { | ||
| // Allowance is 100, and user tries to approve 90, so there should be approval calldata | ||
| await mockServer.forPost('/rpc').thenReply( | ||
| 200, | ||
| JSON.stringify({ | ||
| jsonrpc: '2.0', | ||
| id: 1, | ||
| result: '0x0000000000000000000000000000000000000000000000000000000000000064' | ||
| }) | ||
| ); | ||
|
|
||
| const res = await getVaultApprovalCalldata(mockServer.url + '/rpc', vault1, '90'); | ||
| if (!res.error) assert.fail('expected to reject, but resolved'); | ||
| assert.equal(res.error.msg, 'Invalid amount'); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Good test coverage for insufficient approval amount.
The test correctly verifies that when the requested approval amount (90) is less than the current allowance (100), the function rejects the request. However, there's a minor style issue to address.
Consider using template literals for better readability:
- const res = await getVaultApprovalCalldata(mockServer.url + '/rpc', vault1, '90');
+ const res = await getVaultApprovalCalldata(`${mockServer.url}/rpc`, vault1, '90');Also apply the same fix to line 324:
- const res = await getVaultApprovalCalldata(mockServer.url + '/rpc', vault1, '100');
+ const res = await getVaultApprovalCalldata(`${mockServer.url}/rpc`, vault1, '100');📝 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('should handle approval amount less than allowance', async () => { | |
| // Allowance is 100, and user tries to approve 90, so there should be approval calldata | |
| await mockServer.forPost('/rpc').thenReply( | |
| 200, | |
| JSON.stringify({ | |
| jsonrpc: '2.0', | |
| id: 1, | |
| result: '0x0000000000000000000000000000000000000000000000000000000000000064' | |
| }) | |
| ); | |
| const res = await getVaultApprovalCalldata(mockServer.url + '/rpc', vault1, '90'); | |
| if (!res.error) assert.fail('expected to reject, but resolved'); | |
| assert.equal(res.error.msg, 'Invalid amount'); | |
| }); | |
| it('should handle approval amount less than allowance', async () => { | |
| // Allowance is 100, and user tries to approve 90, so there should be approval calldata | |
| await mockServer.forPost('/rpc').thenReply( | |
| 200, | |
| JSON.stringify({ | |
| jsonrpc: '2.0', | |
| id: 1, | |
| result: '0x0000000000000000000000000000000000000000000000000000000000000064' | |
| }) | |
| ); | |
| const res = await getVaultApprovalCalldata(`${mockServer.url}/rpc`, vault1, '90'); | |
| if (!res.error) assert.fail('expected to reject, but resolved'); | |
| assert.equal(res.error.msg, 'Invalid amount'); | |
| }); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 340-340: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
🤖 Prompt for AI Agents
In packages/orderbook/test/js_api/vault.test.ts around lines 324 and 329 to 343,
the test uses string concatenation or plain strings where template literals
would improve readability. Update the code to use template literals for
constructing strings, especially in the getVaultApprovalCalldata call and any
related string usage, to enhance clarity and maintain consistency.
…ror-ie-no-approval-required-if-deposit-allowance
…ror-ie-no-approval-required-if-deposit-allowance
3 participants
Motivation
Fixes #1900
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
Bug Fixes
Tests