feat(BundleDataClient): Support refunds for pre-fills/slow-fill-requests and duplicate deposits #835
Conversation
…re-slowfill-requests ### Definitions - "pre-fill": any fill that is in a bundle that precedes the bundle containing the matched deposit - "pre-slow-fill-request": a mouthful, and any slow fill request in a bundle that precedes the bundle containing the matched deposit ## Core logic for refunding pre-fills: Evaluate all deposits in the current bundle that were not filled in the current bundle. Figure out whether the deposit was filled, by either: - Finding the fill in the spoke pool client's memory, or - Querying the fill status on-chain If there is a fill for this deposit then we need to issue a refund for this fill because the fill occurred in a previous bundle where it might not have been refunded. We need to use a similar algorithm for creating slow fill leaves for pre-slow-fill-requests ## Avoiding duplicate refunds for pre-fills We don't deterministically know whether this pre-fill was refunded in the prior bundle because we don't know for certain what kind of event search window the proposer of the prior bundle used when constructing the bundle. To illustrate this problem, imagine if a fill and a deposit are sent 10 minutes apart such that the fill is at the end of the current bundle and the deposit is at the beginning of the next. The prior bundle proposer probably would have issued a refund for this fill, but we don't know for certain. So, one way around this non-determinism is to introduce a new rule to the UMIP: ### Do not issue refunds for fills or slow fill leaves where the matched deposit is later than the current bundle block range This rule makes it always apparent which bundle should include a refund or slow fill leaf for a pre-fill or pre-slow-fill-request. This will require a UMIP change to [this PR](UMAprotocol/UMIPs#611) ## TODO I still need to update the `findFillBlock()` function to return a historical fill for a deposit, because we'll need the FilledRelay's `relayerAddress` or `msg.sender` to credit the refund to (the latter in the case where the relayer address isn't valid on the repayment chain).
This PR should be backwards compatible and deployable to production today to be paired with across-protocol/sdk#835
I think these are the changes needed but I need to consider more the impact if two deposits expire and they both create slow fill excesses, for example, or other weird edge cases.
Put more likely to be used filters in conditions first
There was a problem hiding this comment.
I'm not sure the best way to merge this PR. Any bundles produced by this SDK will likely disagree with any existing bundles and therefore could lead to a self-dispute. This is because each bundle has a decent chance empirically of having a deposit slightly after a bundle block range matching a fill at the end of the range. One way to do this is to make sure to merge this in when there are no bundles pending and propose the next bundle with this code.
| // In production the chainId returned from the provider matches 1:1 with the actual chainId. Querying the provider | ||
| // object saves an RPC query becasue the chainId is cached by StaticJsonRpcProvider instances. In hre, the SpokePool | ||
| // may be configured with a different chainId than what is returned by the provider. | ||
| // @todo Sub out actual chain IDs w/ CHAIN_IDs constants |
There was a problem hiding this comment.
You can probably remove this todo comment.
| deposit && | ||
| deposit.originChainId === originChainId && | ||
| deposit.destinationChainId === destinationChainId && | ||
| deposit.blockNumber >= originBlockRange[0] && |
There was a problem hiding this comment.
These last three checks about the block ranges and zero value deposits are redundant since we check them when adding to bundleDepositHashes. Do we want to keep them for the sake of being extra safe (e.g. if we change how we add stuff to bundleDepositHashes in the future)?
There was a problem hiding this comment.
good call will remove
There was a problem hiding this comment.
Do we want to activate this before or after this PR: https://github.com/across-protocol/sdk/pull/831/files? I'd prefer activating this before, since this is significantly more complex.
|
It will be annoying, but we can check this PR by running it in parallel with the dataworker and then manually going through the fill/deposit events and making sure this new BundleDataClient ignores all deposits outside of the block ranges. I don't really see how else we can check reliably. |
|
This makes sense, but the invalid fills PR should only be a pass through for right now. That is, none of the new conditionals will be triggered since all addresses are bytes20 addresses and all chains are EVM. |
agreed but better to just be certain no other unexecpted regressions due to some other bug |
Definitions
Core logic for refunding pre-fills:
Evaluate all deposits in the current bundle that were not filled in the current bundle. Figure out whether the deposit was filled, by either:
If there is a fill for this deposit then we need to issue a refund for this fill because the fill occurred in a previous bundle where it might not have been refunded.
We need to use a similar algorithm for creating slow fill leaves for pre-slow-fill-requests with the caveat that we cannot create slow fill leaves for deposits that have expired
Non-determinism for refunding pre-fills
We don't deterministically know whether this pre-fill was refunded in the prior bundle because we don't know for certain what kind of event search window the proposer of the prior bundle used when constructing the bundle.
To illustrate this problem, imagine if a fill and a deposit are sent 10 minutes apart such that the fill is at the end of the current bundle and the deposit is at the beginning of the next. The prior bundle proposer probably would have issued a refund for this fill, but we don't know for certain.
So, one way around this non-determinism is to introduce a new rule to the UMIP:
Do not issue refunds for fills or slow fill leaves where the matched deposit is later than the current bundle block range
This rule makes it always apparent which bundle should include a refund or slow fill leaf for a pre-fill or pre-slow-fill-request.
This will require a UMIP change to this PR
findFillEvents
When we need to query
fillStatuses()on-chain to detect whether a deposit corresponds to a very old pre-fill, then we also need to locate that old fill. We assume this is rare and will use the relatively expensivefindFillEvents()function which is built off of the existingfindFillBlock()function to locate that event.Duplicate deposits
This PR also adds support for duplicate deposits that are possible to create with unsafeDeposit. Duplicate deposits are added to
bundleDepositsfor they should be included inrunningBalances. Duplicate deposits can be refunded upon expiry but for all deposits, expiry refunds cannot occur once the deposit has been filled.Due to the presence of pre-fills, we can now no longer instantly include that a deposit submitted in the current bundle that has expired is due a refund. We now need to check its relay status in the case where we don't see a matching fill in our memory.
Unit tests
Can be found here: across-protocol/relayer#2010