Skip to content
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.

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

improve(BundleDataClient): Skip queryHistoricalDepositForFill function for non-infinite expiry deposits #1769

Merged
merged 5 commits into from
Aug 26, 2024

Conversation

nicholaspai
Copy link
Member

@nicholaspai nicholaspai commented Aug 20, 2024

This is one step toward phasing out the deposit() method which produces deposits with infinite expiries. Infinite expiries produce extra load on the off-chain systems and once we migrate all deposits to the depositV3() function we likely don't need to use the queryHistoricalDepositForFill function anymore if we have the appropriate checks in place that the spoke pool clients have loaded old enough data for a given bundle block range. This is currently done by the blockRangesAreInvalidForSpokeClients function for example here.

The second motivation for this pr is that if we execute a spoke pool upgrade like this one, the queryHistoricalDeposit() function will no longer be useful for looking up deposits with user supplied relay hashes (because we can't apply the binary search algorithm on the depositId anymore).

The goals long term IMO of the protocol should be:

  • eliminate deposits with infinite fill deadlines and ultimately remove the queryHistoricalDeposit function
  • in the meantime, make sure we're only using this function on infinite fill deadline deposits, which will be mutually exclusive with deposits sent with deterministic relay hashes

…ion for non-infinite expiry deposits

This is one step toward phasing out the `deposit()` method which produces deposits with infinite expiries. Infinite expiries produce extra load on the off-chain systems and once we migrate all deposits to the `depositV3()` function we likely don't need to use the `queryHistoricalDepositForFill` function anymore if we have the appropriate checks in place that the spoke pool clients have loaded old enough data for a given bundle block range. This is currently done by the `blockRangesAreInvalidForSpokeClients` function for example [here](https://github.com/across-protocol/relayer/blob/005c52ae0f4b497afb8d3e359caf4f23a521341f/src/dataworker/Dataworker.ts#L316).
Copy link
Contributor

@james-a-morris james-a-morris left a comment

Choose a reason for hiding this comment

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

lgtm - left a few comments on the code itself. But otherwise this makes sense.

I would be curious to know if this will have unintended effects in our disputer given that we're now classifying additional fills as invalid

src/clients/BundleDataClient.ts Show resolved Hide resolved
src/clients/BundleDataClient.ts Show resolved Hide resolved
@nicholaspai
Copy link
Member Author

nicholaspai commented Aug 24, 2024

lgtm - left a few comments on the code itself. But otherwise this makes sense.

I would be curious to know if this will have unintended effects in our disputer given that we're now classifying additional fills as invalid

I don't think we're classifying additional fills as invalid. We're just assuming a fill is invalid if its fill deadline is not infinite and we haven't found the deposit in the lookback window. Compare that with the current logic where we try to look for the deposit using a binary search. This doesn't happen that often in product currently because usually the depositId of an invalid fill corresponds to a legit deposit, but the fill is wrong for other reasons

This PR actually doesn't improve performance that much currently but I think it could be very important after an upgrade like this one which will make the queryHistoricalDeposit() function useless for looking up deposits with user supplied relay hashes (because we can't apply the binary search algorithm on the depositId anymore).

Copy link
Contributor

@james-a-morris james-a-morris left a comment

Choose a reason for hiding this comment

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

I've left a few nits but otherwise LGTM

@nicholaspai nicholaspai merged commit aa1ec0d into master Aug 26, 2024
4 checks passed
bmzig pushed a commit that referenced this pull request Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants