-
Notifications
You must be signed in to change notification settings - Fork 466
Conversation
acaeec0
to
8ffceb0
Compare
fe109b1
to
31ea30f
Compare
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.
Looks good, happy to get away from using the addresses as id's. Seems clear the changes for PLP and other sources. Much easier to add a */WETH sample than having to perform 2 calls simultaneously.
// prettier-ignore | ||
public async executeAsync< | ||
T1, T2, T3, T4, T5, T6, T7, T8 | ||
>(...ops: [T1, T2, T3, T4, T5, T6, T7, T8]): Promise<[ |
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.
😢
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.
okie dokie to me 👍
nativeOrders, | ||
DexOrderSampler.getSampleAmounts(takerAmount, _opts.numSamples, _opts.sampleDistributionBase), | ||
difference(SELL_SOURCES, _opts.excludedSources), | ||
const [makerToken, takerToken] = getOrderTokens(nativeOrders[0]); |
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.
nit: you've got const [outputToken, inputToken] = getOrderTokens(nativeOrders[0]);
later in this function
function createOrder(overrides?: Partial<SignedOrder>): SignedOrder { | ||
return { | ||
chainId: CHAIN_ID, | ||
exchangeAddress: hexUtils.random(20), |
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.
why no randomAddress
?
}); | ||
|
||
it('getKyberSellQuotes()', async () => { | ||
const expectedTakerToken = hexUtils.random(20); |
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.
ditto
…troduce `batchCall()`.
…innet and kovan.
31ea30f
to
a36ff9e
Compare
Description
We kept adding wrapper functions upon wrapper functions to
ERC20BridgeSampler
as our needs evolved while keeping the RPC calls to the bare minimum, which resulted in a bloated mess.This PR stems tech debt by removing all the wrapper functions, leaving only atomic ones, and introducing a
batchCall()
function that:batchCall()
again to recurse.The upshot is this allows us to compose function calls off-chain instead of adding higher order wrapper functions to and redeploying the sampler contract. This is going to be relevant to fees/gas accounting because we will probably need to sample pairs and quantities that aren't involved in a swap to establish a baseline. Otherwise, we'd be adding some stupid sounding function like
getEthPriceAndFillableAmountsAndSampleMarketSell()
to the contract 😫.DexOrderSampler
The
DexOrderSampler
class has been rewritten to be a convenient wrapper for neatly performing an arbitrary number of batch operations. So instead of doing this:We now do this:
Which is much clearer and scalable, imo.
Testing instructions
Types of changes
Checklist:
[WIP]
if necessary.