-
Notifications
You must be signed in to change notification settings - Fork 465
Make it easier to use validateOrderFillableOrThrowAsync #2096
Changes from all commits
31aa55f
7eae251
3bb5446
6d7f786
3776e31
eae90e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
import { DevUtilsContract } from '@0x/abi-gen-wrappers'; | ||
import { BigNumber } from '@0x/utils'; | ||
import * as _ from 'lodash'; | ||
|
||
import { AbstractBalanceAndProxyAllowanceFetcher } from './abstract/abstract_balance_and_proxy_allowance_fetcher'; | ||
|
||
export class AssetBalanceAndProxyAllowanceFetcher implements AbstractBalanceAndProxyAllowanceFetcher { | ||
private readonly _devUtilsContract: DevUtilsContract; | ||
constructor(devUtilsContract: DevUtilsContract) { | ||
this._devUtilsContract = devUtilsContract; | ||
} | ||
public async getBalanceAsync(assetData: string, userAddress: string): Promise<BigNumber> { | ||
const balance = await this._devUtilsContract.getBalance.callAsync(userAddress, assetData); | ||
return balance; | ||
} | ||
public async getProxyAllowanceAsync(assetData: string, userAddress: string): Promise<BigNumber> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should be able to refactor this to use DevUtils.
Which handles ERC20, ERC721, ERC1155 and MAP. |
||
const proxyAllowance = await this._devUtilsContract.getAssetProxyAllowance.callAsync(userAddress, assetData); | ||
return proxyAllowance; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,29 @@ | ||
import { | ||
DevUtilsContract, | ||
ExchangeContract, | ||
getContractAddressesForNetworkOrThrow, | ||
IAssetProxyContract, | ||
NetworkId, | ||
} from '@0x/abi-gen-wrappers'; | ||
import { assert } from '@0x/assert'; | ||
import { getNetworkIdByExchangeAddressOrThrow } from '@0x/contract-addresses'; | ||
import { ExchangeContractErrs, RevertReason, SignedOrder } from '@0x/types'; | ||
import { BigNumber, providerUtils } from '@0x/utils'; | ||
import { SupportedProvider, ZeroExProvider } from 'ethereum-types'; | ||
import * as _ from 'lodash'; | ||
|
||
import { AbstractOrderFilledCancelledFetcher } from './abstract/abstract_order_filled_cancelled_fetcher'; | ||
import { AssetBalanceAndProxyAllowanceFetcher } from './asset_balance_and_proxy_allowance_fetcher'; | ||
import { assetDataUtils } from './asset_data_utils'; | ||
import { constants } from './constants'; | ||
import { ExchangeTransferSimulator } from './exchange_transfer_simulator'; | ||
import { orderCalculationUtils } from './order_calculation_utils'; | ||
import { orderHashUtils } from './order_hash'; | ||
import { OrderStateUtils } from './order_state_utils'; | ||
import { validateOrderFillableOptsSchema } from './schemas/validate_order_fillable_opts_schema'; | ||
import { signatureUtils } from './signature_utils'; | ||
import { TradeSide, TransferType, TypedDataError } from './types'; | ||
import { BalanceAndProxyAllowanceLazyStore } from './store/balance_and_proxy_allowance_lazy_store'; | ||
import { TradeSide, TransferType, TypedDataError, ValidateOrderFillableOpts } from './types'; | ||
import { utils } from './utils'; | ||
|
||
/** | ||
|
@@ -171,6 +179,74 @@ export class OrderValidationUtils { | |
const provider = providerUtils.standardizeOrThrow(supportedProvider); | ||
this._provider = provider; | ||
} | ||
|
||
// TODO(xianny): remove this method once the smart contracts have been refactored | ||
// to return helpful revert reasons instead of ORDER_UNFILLABLE. Instruct devs | ||
// to make "calls" to validate order fillability + getOrderInfo for fillable amount. | ||
// This method recreates functionality from ExchangeWrapper (@0x/contract-wrappers < 11.0.0) | ||
// to make migrating easier in the interim. | ||
/** | ||
* Validate if the supplied order is fillable, and throw if it isn't | ||
* @param provider The same provider used to interact with contracts | ||
* @param signedOrder SignedOrder of interest | ||
* @param opts ValidateOrderFillableOpts options (e.g expectedFillTakerTokenAmount. | ||
* If it isn't supplied, we check if the order is fillable for the remaining amount. | ||
* To check if the order is fillable for a non-zero amount, set `validateRemainingOrderAmountIsFillable` to false.) | ||
*/ | ||
public async simpleValidateOrderFillableOrThrowAsync( | ||
provider: SupportedProvider, | ||
signedOrder: SignedOrder, | ||
opts: ValidateOrderFillableOpts = {}, | ||
): Promise<void> { | ||
assert.doesConformToSchema('opts', opts, validateOrderFillableOptsSchema); | ||
|
||
const exchangeAddress = signedOrder.exchangeAddress; | ||
const networkId = getNetworkIdByExchangeAddressOrThrow(exchangeAddress); | ||
const { zrxToken, devUtils } = getContractAddressesForNetworkOrThrow(networkId); | ||
const exchangeContract = new ExchangeContract(exchangeAddress, provider); | ||
const balanceAllowanceFetcher = new AssetBalanceAndProxyAllowanceFetcher( | ||
new DevUtilsContract(devUtils, provider), | ||
); | ||
const balanceAllowanceStore = new BalanceAndProxyAllowanceLazyStore(balanceAllowanceFetcher); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I do not believe DevUtils currently handles this edge case. Extrapolating to V3. I'm selling a Kitty for 100 WETH, there is a 1 WETH fee. I have 0 WETH. This order is valid as there will be funds for fees during settlement but not prior to settlement. |
||
const exchangeTradeSimulator = new ExchangeTransferSimulator(balanceAllowanceStore); | ||
|
||
// Define fillable taker asset amount | ||
let fillableTakerAssetAmount; | ||
const shouldValidateRemainingOrderAmountIsFillable = | ||
opts.validateRemainingOrderAmountIsFillable === undefined | ||
? true | ||
: opts.validateRemainingOrderAmountIsFillable; | ||
if (opts.expectedFillTakerTokenAmount) { | ||
// If the caller has specified a taker fill amount, we use this for all validation | ||
fillableTakerAssetAmount = opts.expectedFillTakerTokenAmount; | ||
} else if (shouldValidateRemainingOrderAmountIsFillable) { | ||
// Default behaviour is to validate the amount left on the order. | ||
const filledTakerTokenAmount = await exchangeContract.filled.callAsync( | ||
orderHashUtils.getOrderHashHex(signedOrder), | ||
); | ||
fillableTakerAssetAmount = signedOrder.takerAssetAmount.minus(filledTakerTokenAmount); | ||
} else { | ||
const orderStateUtils = new OrderStateUtils(balanceAllowanceStore, this._orderFilledCancelledFetcher); | ||
// Calculate the taker amount fillable given the maker balance and allowance | ||
const orderRelevantState = await orderStateUtils.getOpenOrderRelevantStateAsync(signedOrder); | ||
fillableTakerAssetAmount = orderRelevantState.remainingFillableTakerAssetAmount; | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. above boilerplate retrieved from https://github.com/0xProject/0x-monorepo/pull/2037/files#diff-540ab868df58adcdec38223c40efc2e1L1160-L1188 |
||
await this.validateOrderFillableOrThrowAsync( | ||
exchangeTradeSimulator, | ||
signedOrder, | ||
assetDataUtils.encodeERC20AssetData(zrxToken), | ||
fillableTakerAssetAmount, | ||
); | ||
const makerTransferAmount = orderCalculationUtils.getMakerFillAmount(signedOrder, fillableTakerAssetAmount); | ||
await OrderValidationUtils.validateMakerTransferThrowIfInvalidAsync( | ||
networkId, | ||
provider, | ||
signedOrder, | ||
makerTransferAmount, | ||
opts.simulationTakerAddress, | ||
); | ||
} | ||
// TODO(fabio): remove this method once the smart contracts have been refactored | ||
// to return helpful revert reasons instead of ORDER_UNFILLABLE. Instruct devs | ||
// to make "calls" to validate order fillability + getOrderInfo for fillable amount. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
export const validateOrderFillableOptsSchema = { | ||
id: '/ValidateOrderFillableOpts', | ||
properties: { | ||
expectedFillTakerTokenAmount: { $ref: '/wholeNumberSchema' }, | ||
}, | ||
type: 'object', | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,7 @@ describe('ExchangeTransferSimulator', async () => { | |
from: devConstants.TESTRPC_FIRST_ADDRESS, | ||
}; | ||
|
||
await blockchainLifecycle.startAsync(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was affecting the new unit tests in |
||
const erc20Proxy = await ERC20ProxyContract.deployFrom0xArtifactAsync( | ||
artifacts.ERC20Proxy, | ||
provider, | ||
|
@@ -74,6 +75,9 @@ describe('ExchangeTransferSimulator', async () => { | |
afterEach(async () => { | ||
await blockchainLifecycle.revertAsync(); | ||
}); | ||
after(async () => { | ||
await blockchainLifecycle.revertAsync(); | ||
}); | ||
describe('#transferFromAsync', function(): void { | ||
// HACK: For some reason these tests need a slightly longer timeout | ||
const mochaTestTimeoutMs = 3000; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to introduce a dependency on
@0x/migrations
to@0x/order-utils
for tests (which were retrieved from the old exchange wrapper tests). I've run into similar issues with tests introducing circular dependencies for other packages. Would like to discuss the possibility of developing an integration test suite for any tests that run with ganache and the standard 0x migration. Two benefits: 1) potentially speed up our testing time, by doing one migration + snapshots instead of separate migrations in numerous unit test suites, and 2) reduce package size/unnecessary dependency trees in the published packages.Since we intend to remove these methods in a couple months, this seems like the easiest solution for now to unblock this task.