Revert "fix: Fixes allowances issue with limit orders"#7656
Conversation
…g sell a…" This reverts commit be0d4c7.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Walkthrough
ChangesAllowance/Balance Sufficiency Simplification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Deploying swap-dev with
|
| Latest commit: |
530a006
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://65ff2def.swap-dev-5u6.pages.dev |
| Branch Preview URL: | https://revert-7614-fix-limit-allowa.swap-dev-5u6.pages.dev |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cowswap-frontend/src/modules/ordersTable/utils/getOrderParams.test.ts`:
- Around line 13-16: In the getOrderParams.test.ts file, replace all direct uses
of `.toLowerCase()` on address properties with the `getAddressKey()` function
from `@cowprotocol/cow-sdk` to maintain consistency with production code.
Specifically, update all occurrences where
`BASE_ORDER.inputToken.address.toLowerCase()` is used as a key in map objects
(such as in the balances and allowances objects) to instead use
`getAddressKey(BASE_ORDER.inputToken.address)`. Make sure to import
`getAddressKey` if it is not already imported, and apply this pattern
consistently throughout the test file wherever addresses are used as object
keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1ecece6f-9e54-4e21-9532-8172b071ee11
📒 Files selected for processing (2)
apps/cowswap-frontend/src/modules/ordersTable/utils/getOrderParams.test.tsapps/cowswap-frontend/src/modules/ordersTable/utils/getOrderParams.ts
| [BASE_ORDER.inputToken.address.toLowerCase()]: BigInt(BASE_ORDER.sellAmount), | ||
| }, | ||
| allowances: { | ||
| [getAddressKey(BASE_ORDER.inputToken.address)]: BigInt(BASE_ORDER.sellAmount), | ||
| [BASE_ORDER.inputToken.address.toLowerCase()]: BigInt(BASE_ORDER.sellAmount), |
There was a problem hiding this comment.
Use getAddressKey instead of toLowerCase() for address keys.
The test creates map keys with .toLowerCase() while the production code uses getAddressKey() for lookups. This violates the coding guideline and creates implicit coupling to getAddressKey's implementation details.
Proposed fix
+import { getAddressKey } from '`@cowprotocol/cow-sdk`'
import { BalancesAndAllowances } from '`@cowprotocol/balances-and-allowances`'
import { getOrderParams } from './getOrderParams'Then replace all occurrences of inputToken.address.toLowerCase() with getAddressKey(inputToken.address):
const BASE_BALANCES_AND_ALLOWANCES: BalancesAndAllowances = {
balances: {
- [BASE_ORDER.inputToken.address.toLowerCase()]: BigInt(BASE_ORDER.sellAmount),
+ [getAddressKey(BASE_ORDER.inputToken.address)]: BigInt(BASE_ORDER.sellAmount),
},
allowances: {
- [BASE_ORDER.inputToken.address.toLowerCase()]: BigInt(BASE_ORDER.sellAmount),
+ [getAddressKey(BASE_ORDER.inputToken.address)]: BigInt(BASE_ORDER.sellAmount),
},Apply the same pattern to lines 70, 81, 93, and 104.
As per coding guidelines: "MUST NOT normalize addresses with address.toLowerCase(); use getAddressKey from @cowprotocol/cow-sdk"
📝 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.
| [BASE_ORDER.inputToken.address.toLowerCase()]: BigInt(BASE_ORDER.sellAmount), | |
| }, | |
| allowances: { | |
| [getAddressKey(BASE_ORDER.inputToken.address)]: BigInt(BASE_ORDER.sellAmount), | |
| [BASE_ORDER.inputToken.address.toLowerCase()]: BigInt(BASE_ORDER.sellAmount), | |
| [getAddressKey(BASE_ORDER.inputToken.address)]: BigInt(BASE_ORDER.sellAmount), | |
| }, | |
| allowances: { | |
| [getAddressKey(BASE_ORDER.inputToken.address)]: BigInt(BASE_ORDER.sellAmount), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/cowswap-frontend/src/modules/ordersTable/utils/getOrderParams.test.ts`
around lines 13 - 16, In the getOrderParams.test.ts file, replace all direct
uses of `.toLowerCase()` on address properties with the `getAddressKey()`
function from `@cowprotocol/cow-sdk` to maintain consistency with production code.
Specifically, update all occurrences where
`BASE_ORDER.inputToken.address.toLowerCase()` is used as a key in map objects
(such as in the balances and allowances objects) to instead use
`getAddressKey(BASE_ORDER.inputToken.address)`. Make sure to import
`getAddressKey` if it is not already imported, and apply this pattern
consistently throughout the test file wherever addresses are used as object
keys.
Source: Coding guidelines
3 participants
This reverts commit be0d4c7.
Summary
Switch back to the old behaviour where only 0.05% of the original sell amount is needed in order to not trigger the inssuficient allowance warning.
To Test
Background
Slack conversation: https://nomevlabs.slack.com/archives/C0361CDG8GP/p1781601276928229
Summary by CodeRabbit
Refactor
Tests