fix(FillUtils): verifyFillRepayment should check that repayment chain has pool rebalance route #863
Conversation
… has pool rebalance route We cannot refund fills whose repayment chain doesn't exist for an input token.
mrice32
force-pushed
the
fix-verify-fill-repayment
branch
from
7791cb7 to
7a8872d
Compare
| hubPoolClient.getL2TokenForL1TokenAtBlock(l1TokenCounterpart, repaymentChainId, matchedDeposit.quoteBlockNumber); | ||
| // Repayment token could be found, this is a valid repayment chain. | ||
| } catch { | ||
| // Repayment token doesn't exist on repayment chain via PoolRebalanceRoutes, impossible to repay filler. |
There was a problem hiding this comment.
Do we want to log this via unpayable-repayments? The HubPoolClient has a logger instance you could piggyback on.
| ): Promise<FillWithBlock | undefined> { | ||
| const fill = _.cloneDeep(_fill); | ||
|
|
||
| const repaymentChainId = getRepaymentChainId(fill, matchedDeposit); | ||
| const validEvmRepayment = isEvmRepaymentValid(fill, repaymentChainId, possibleRepaymentChainIds); | ||
| const validEvmRepayment = isEvmRepaymentValid(fill, repaymentChainId); | ||
| if (!isSlowFill(fill)) { |
There was a problem hiding this comment.
Should we be calling verifyFillRepayment() on slow fills?
There was a problem hiding this comment.
I think it depends where you want to make the isSlowFill check. Currently we call verifyFillRepayment before isSlowFill so this is best for reducing diff in the BundleDataClient for now
| try { | ||
| const l1TokenCounterpart = hubPoolClient.getL1TokenForL2TokenAtBlock( | ||
| fill.inputToken, | ||
| fill.originChainId, | ||
| matchedDeposit.quoteBlockNumber | ||
| ); | ||
| hubPoolClient.getL2TokenForL1TokenAtBlock(l1TokenCounterpart, repaymentChainId, matchedDeposit.quoteBlockNumber); | ||
| // Repayment token could be found, this is a valid repayment chain. | ||
| } catch { |
There was a problem hiding this comment.
This universal try catch is quite dangerous. Should we fix this now or in a follow up?
One way to mostly fix it is to make the hub error a specific type and catch only that werror type.
There was a problem hiding this comment.
changed behavior to switch to destination chain
There was a problem hiding this comment.
Still dangerous as a bug will silently change repayment location, causing invalid bundles.
We can resolve in follow up.
There was a problem hiding this comment.
but wouldn't that indicate the bug that causes the invalid bundles is an invalid code change?
There was a problem hiding this comment.
We want an error in an inner function to bubble up, not silently create bad bundles.
My point is that we're assuming the error is X when we could be catching error Y without realizing it.
There was a problem hiding this comment.
This universal try catch is quite dangerous. Should we fix this now or in a follow up?
One way to mostly fix it is to make the hub error a specific type and catch only that werror type.
We started on this a while ago and should pick it up again: #643
| } | ||
|
|
||
| export function isEvmRepaymentValid( | ||
| fill: Fill, | ||
| export function willOverwriteRepaymentChain( |
There was a problem hiding this comment.
nit: I'd probably prefer something like forceDestinationRepayment since willOverwriteRepaymentChain seems a bit ambiguous.
There was a problem hiding this comment.
i like that its more precise
| const fill = await verifyFillRepayment( | ||
| _fill, | ||
| this.spokePoolClients[_fill.destinationChainId].spokePool.provider, | ||
| matchingDeposit!, |
There was a problem hiding this comment.
Driveby question - any idea why it's necessary to assert the existence of matchingDeposit here when the check above should guarantee it?
There was a problem hiding this comment.
I think because TSC doesn't interpret asserts the same way it does if statements. We could change the above assert to an if statement to avoid this "!"
There was a problem hiding this comment.
I figured it out - it's because we have some locally-defined assert that instead throws an exception.
This diff fixes it so that tsc can infer things from the use of asserts:
diff --git a/src/clients/BundleDataClient/BundleDataClient.ts b/src/clients/BundleDataClient/BundleDataClient.ts
index cb0f4332..d9dc2469 100644
--- a/src/clients/BundleDataClient/BundleDataClient.ts
+++ b/src/clients/BundleDataClient/BundleDataClient.ts
@@ -1,3 +1,4 @@
+import assert from "assert";
import _ from "lodash";
import {
ProposedRootBundle,
@@ -24,7 +25,6 @@ import {
bnZero,
queryHistoricalDepositForFill,
assign,
- assert,
fixedPointAdjustment,
isDefined,
toBN,
@@ -371,7 +371,7 @@ export class BundleDataClient {
const fill = await verifyFillRepayment(
_fill,
this.spokePoolClients[_fill.destinationChainId].spokePool.provider,
- matchingDeposit!,
+ matchingDeposit,
this.clients.hubPoolClient
);
if (!isDefined(fill)) {
~
… has pool rebalance route (#863)
We cannot refund fills whose repayment chain doesn't exist for an input token.