Skip to content

Revert "fix: Fixes allowances issue with limit orders" #7656

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Danziger wants to merge 1 commit into
base: develop
Choose a base branch
from
+19 −175
Open

Revert "fix: Fixes allowances issue with limit orders" #7656

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
147 changes: 8 additions & 139 deletions apps/cowswap-frontend/src/modules/ordersTable/utils/getOrderParams.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,47 +1,19 @@
import { BalancesAndAllowances } from '@cowprotocol/balances-and-allowances'
import { getAddressKey } from '@cowprotocol/cow-sdk'

import BigNumber from 'bignumber.js'
import JSBI from 'jsbi'

import { ParsedOrder } from 'utils/orderUtils/parseOrder'

import { getOrderParams } from './getOrderParams'

import { ordersMock } from '../test/ordersTable.mock'

function createPartiallyExecutedOrder(
baseOrder: ParsedOrder,
params: { sellAmount: string; executedSellAmount: string; executedBuyAmount?: string },
): ParsedOrder {
const { sellAmount, executedSellAmount, executedBuyAmount = '1' } = params
const filledAmount = new BigNumber(executedSellAmount)

return {
...baseOrder,
sellAmount,
executionData: {
...baseOrder.executionData,
executedSellAmount: JSBI.BigInt(executedSellAmount),
executedBuyAmount: JSBI.BigInt(executedBuyAmount),
filledAmount,
filledPercentage: filledAmount.div(new BigNumber(sellAmount)),
partiallyFilled: true,
fullyFilled: false,
},
}
}

// TODO: Break down this large function into smaller functions

describe('getOrderParams', () => {
const BASE_ORDER = ordersMock[0]
const BASE_BALANCES_AND_ALLOWANCES: BalancesAndAllowances = {
balances: {
[getAddressKey(BASE_ORDER.inputToken.address)]: BigInt(BASE_ORDER.sellAmount),
[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),
Comment on lines +13 to +16

@coderabbitai coderabbitai Bot Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Suggested change
[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

},
isLoading: false,
}
Expand All @@ -51,7 +23,6 @@ describe('getOrderParams', () => {
...BASE_ORDER,
partiallyFillable: false,
}

it('should have hasEnoughBalance true when order is fill or kill and balance is sufficient', () => {
const order = { ...FILL_OR_KILL_ORDER }
const balancesAndAllowances: BalancesAndAllowances = { ...BASE_BALANCES_AND_ALLOWANCES }
Expand All @@ -75,7 +46,6 @@ describe('getOrderParams', () => {
const result = getOrderParams(1, balancesAndAllowances, order)
expect(result.hasEnoughAllowance).toEqual(true)
})

it('should have hasEnoughAllowance false when order is fill or kill and allowance is insufficient', () => {
const order = {
...FILL_OR_KILL_ORDER,
Expand All @@ -92,151 +62,50 @@ describe('getOrderParams', () => {
...BASE_ORDER,
partiallyFillable: true,
}

it('should have hasEnoughBalance true when order is partially fillable and balance is > 0.05%', () => {
const order = { ...PARTIALLY_FILLABLE_ORDER }
const balancesAndAllowances: BalancesAndAllowances = {
...BASE_BALANCES_AND_ALLOWANCES,
balances: {
[getAddressKey(order.inputToken.address)]: BigInt(String(+order.sellAmount * 0.00051)),
[order.inputToken.address.toLowerCase()]: BigInt(String(+order.sellAmount * 0.00051)),
},
}
const result = getOrderParams(1, balancesAndAllowances, order)
expect(result.hasEnoughBalance).toEqual(true)
})

it('should have hasEnoughBalance false when order is partially fillable and balance is < 0.05%', () => {
const order = { ...PARTIALLY_FILLABLE_ORDER }
const balancesAndAllowances: BalancesAndAllowances = {
...BASE_BALANCES_AND_ALLOWANCES,
balances: {
[getAddressKey(order.inputToken.address)]: BigInt(String(+order.sellAmount * 0.00049)),
[order.inputToken.address.toLowerCase()]: BigInt(String(+order.sellAmount * 0.00049)),
},
}
const result = getOrderParams(1, balancesAndAllowances, order)
expect(result.hasEnoughBalance).toEqual(false)
})

it('should have hasEnoughAllowance true when order is partially fillable and allowance covers full sell amount', () => {
it('should have hasEnoughAllowance true when order is partially fillable and allowance is > 0.05%', () => {
const order = { ...PARTIALLY_FILLABLE_ORDER }
const balancesAndAllowances: BalancesAndAllowances = {
...BASE_BALANCES_AND_ALLOWANCES,
allowances: {
[getAddressKey(order.inputToken.address)]: BigInt(order.sellAmount),
[order.inputToken.address.toLowerCase()]: BigInt(String(+order.sellAmount * 0.00051)),
},
}
const result = getOrderParams(1, balancesAndAllowances, order)
expect(result.hasEnoughAllowance).toEqual(true)
})

it('should have hasEnoughAllowance false when order is partially fillable and allowance is below full sell amount', () => {
it('should have hasEnoughAllowance false when order is partially fillable and allowance is < 0.05%', () => {
const order = { ...PARTIALLY_FILLABLE_ORDER }
const balancesAndAllowances: BalancesAndAllowances = {
...BASE_BALANCES_AND_ALLOWANCES,
allowances: {
[getAddressKey(order.inputToken.address)]: BigInt(String(+order.sellAmount * 0.5)),
},
}
const result = getOrderParams(1, balancesAndAllowances, order)
expect(result.hasEnoughAllowance).toEqual(false)
})

it('should have hasEnoughAllowance false when order is partially fillable and allowance is dust only', () => {
const order = { ...PARTIALLY_FILLABLE_ORDER }
const balancesAndAllowances: BalancesAndAllowances = {
...BASE_BALANCES_AND_ALLOWANCES,
allowances: {
[getAddressKey(order.inputToken.address)]: BigInt(String(+order.sellAmount * 0.00049)),
},
}
const result = getOrderParams(1, balancesAndAllowances, order)
expect(result.hasEnoughAllowance).toEqual(false)
})

it('should have hasEnoughAllowance true when partially filled and allowance is greater than the remainder', () => {
const order = createPartiallyExecutedOrder(PARTIALLY_FILLABLE_ORDER, {
sellAmount: '1000',
executedSellAmount: '600',
})
const balancesAndAllowances: BalancesAndAllowances = {
...BASE_BALANCES_AND_ALLOWANCES,
allowances: {
[getAddressKey(order.inputToken.address)]: 500n,
},
}
const result = getOrderParams(1, balancesAndAllowances, order)
expect(result.hasEnoughAllowance).toEqual(true)
})

it('should have hasEnoughAllowance false when partially filled and allowance is below the remainder', () => {
const order = createPartiallyExecutedOrder(PARTIALLY_FILLABLE_ORDER, {
sellAmount: '1000',
executedSellAmount: '600',
})
const balancesAndAllowances: BalancesAndAllowances = {
...BASE_BALANCES_AND_ALLOWANCES,
allowances: {
[getAddressKey(order.inputToken.address)]: 300n,
[order.inputToken.address.toLowerCase()]: BigInt(String(+order.sellAmount * 0.00049)),
},
}
const result = getOrderParams(1, balancesAndAllowances, order)
expect(result.hasEnoughAllowance).toEqual(false)
})

it('should have hasEnoughAllowance true when partially filled and allowance equals the remainder exactly', () => {
const order = createPartiallyExecutedOrder(PARTIALLY_FILLABLE_ORDER, {
sellAmount: '1000',
executedSellAmount: '600',
})
const balancesAndAllowances: BalancesAndAllowances = {
...BASE_BALANCES_AND_ALLOWANCES,
allowances: {
[getAddressKey(order.inputToken.address)]: 400n,
},
}
const result = getOrderParams(1, balancesAndAllowances, order)
expect(result.hasEnoughAllowance).toEqual(true)
})
})

describe('missing balances/allowances (not loaded yet)', () => {
it('should have hasEnoughBalance undefined when the balance is not available', () => {
const order = { ...BASE_ORDER }
const balancesAndAllowances: BalancesAndAllowances = {
balances: {},
allowances: {
[getAddressKey(order.inputToken.address)]: BigInt(order.sellAmount),
},
isLoading: true,
}
const result = getOrderParams(1, balancesAndAllowances, order)
expect(result.hasEnoughBalance).toBeUndefined()
})

it('should have hasEnoughAllowance undefined when the allowance map is missing the token', () => {
const order = { ...BASE_ORDER }
const balancesAndAllowances: BalancesAndAllowances = {
balances: {
[getAddressKey(order.inputToken.address)]: BigInt(order.sellAmount),
},
allowances: {},
isLoading: true,
}
const result = getOrderParams(1, balancesAndAllowances, order)
expect(result.hasEnoughAllowance).toBeUndefined()
})

it('should have hasEnoughAllowance undefined when allowances are not loaded', () => {
const order = { ...BASE_ORDER }
const balancesAndAllowances: BalancesAndAllowances = {
balances: {
[getAddressKey(order.inputToken.address)]: BigInt(order.sellAmount),
},
allowances: undefined,
isLoading: true,
}
const result = getOrderParams(1, balancesAndAllowances, order)
expect(result.hasEnoughAllowance).toBeUndefined()
})
})
})
47 changes: 11 additions & 36 deletions apps/cowswap-frontend/src/modules/ordersTable/utils/getOrderParams.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
import { BalancesAndAllowances } from '@cowprotocol/balances-and-allowances'
import { isEnoughAmount } from '@cowprotocol/common-utils'
import { getAddressKey, type SupportedChainId } from '@cowprotocol/cow-sdk'
import { getAddressKey, SupportedChainId } from '@cowprotocol/cow-sdk'
import { Currency, CurrencyAmount, Percent, Token } from '@cowprotocol/currency'

import { getRemainderAmountsWithoutSurplus } from 'legacy/state/orders/utils'

import { RateInfoParams } from 'common/pure/RateInfo'
import { getOrderPermitAmount } from 'utils/orderUtils/getOrderPermitAmount'
import { ParsedOrder } from 'utils/orderUtils/parseOrder'
Expand All @@ -31,7 +29,6 @@ export function getOrderParams(
const isOrderAtLeastOnceFilled = order.executionData.filledAmount.gt(0)
const sellAmount = CurrencyAmount.fromRawAmount(order.inputToken, order.sellAmount)
const buyAmount = CurrencyAmount.fromRawAmount(order.outputToken, order.buyAmount)

const isPermitInvalid = pendingOrdersPermitValidityState
? pendingOrdersPermitValidityState[order.id] === false
: false
Expand All @@ -49,20 +46,12 @@ export function getOrderParams(
const balance = balances[getAddressKey(order.inputToken.address)]
const allowance = allowances?.[getAddressKey(order.inputToken.address)]

// After the first fill, the permit is spent, so we only use the on-chain allowance from then on.
// Before the first fill, we use the bigger of the on-chain approve allowance, or permit amount (once validated on-chain).
const effectiveAllowance = isOrderAtLeastOnceFilled ? allowance : getBiggerAmount(allowance, permitAmount)

const sellAmountRemaining = isOrderAtLeastOnceFilled
? CurrencyAmount.fromRawAmount(order.inputToken, getRemainderAmountsWithoutSurplus(order).sellAmount)
: sellAmount

const { hasEnoughBalance, hasEnoughAllowance } = _hasEnoughBalanceAndAllowance({
balance,
allowance: effectiveAllowance,
partiallyFillable: order.partiallyFillable,
sellAmount,
sellAmountRemaining,
balance,
// If the order has been filled at least once, we should not consider the permit amount
allowance: !isOrderAtLeastOnceFilled ? getBiggerAmount(allowance, permitAmount) : allowance,
})

return {
Expand All @@ -75,34 +64,20 @@ export function getOrderParams(
}
}

interface HasEnoughBalanceAndAllowanceParams {
function _hasEnoughBalanceAndAllowance(params: {
balance: bigint | undefined
allowance: bigint | undefined
partiallyFillable: boolean
sellAmount: CurrencyAmount<Token>
sellAmountRemaining: CurrencyAmount<Token>
}

function _hasEnoughBalanceAndAllowance({
balance,
allowance,
partiallyFillable,
sellAmount,
sellAmountRemaining,
}: HasEnoughBalanceAndAllowanceParams): {
}): {
hasEnoughBalance: boolean | undefined
hasEnoughAllowance: boolean | undefined
} {
// For partially fillable orders, we want to check there's at least `PERCENTAGE_FOR_PARTIAL_FILLS` of balance instead
// of the full amount:
const balanceAmount = partiallyFillable ? sellAmount.multiply(PERCENTAGE_FOR_PARTIAL_FILLS) : sellAmount
const hasEnoughBalance = isEnoughAmount(balanceAmount, balance)

// However, for allowance, we probably want to check if the allowance covers the full sell amount remaining:
const hasEnoughAllowance = isEnoughAmount(sellAmountRemaining, allowance)

// Otherwise, a fillable order with full balance available but just some dust allowance would be considered fillable,
// and the user would falsely expect it to be fully fillable, which would not be the case.
const { allowance, balance, partiallyFillable, sellAmount } = params
// Check there's at least PERCENTAGE_FOR_PARTIAL_FILLS of balance/allowance to consider it as enough
const amount = partiallyFillable ? sellAmount.multiply(PERCENTAGE_FOR_PARTIAL_FILLS) : sellAmount
const hasEnoughBalance = isEnoughAmount(amount, balance)
const hasEnoughAllowance = isEnoughAmount(amount, allowance)

return { hasEnoughBalance, hasEnoughAllowance }
}
Expand Down
Loading