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

feat: Propagate invalid fill reasons to caller #486

Merged
merged 8 commits into from
Jan 4, 2024
Merged

Conversation

pxrl
Copy link
Contributor

@pxrl pxrl commented Jan 2, 2024

This change provides additional information to the caller of queryHistoricalDepositForFill(). In the case that a fill is not found, a cause code is associated:

  • 0: The deposit ID was invalid for the SpokePool (i.e. cannot exist).
  • 1: The deposit ID was valid, but it wasn't found within scraped data (i.e. maybe the RPCs aren't serving it in eth_getLogs() requests).
  • 2: The deposit ID was valid, but one or more fields in the corresponding deposit were mismatched (i.e. the fill was invalid).

This is most interesting for case (1), where there's a reasonable indication that the deposit should be held within the SpokePoolClient's deposit map, but isn't. This indirectly acts like a qualitative measure for RPC provider responses and will allow the upper layers to dynamically revert to more conservative behaviour.

This change provides additional information to the caller of
queryHistoricalDepositForFill(). In the case that a fill is not found, a
cause code is associated:
- 0: The deposit ID was invalid for the SpokePool (i.e. cannot exist).
- 1: The deposit ID was valid, but it wasn't found within scraped data
  (i.e. maybe the RPCs aren't serving it in eth_getLogs() requests).
- 2: The deposit ID was valid, but one or more fields in the
  corresponding deposit were mismatched (i.e. the fill was invalid).

This is most interesting for case (1), where there's a reasonable
indication that the deposit should be held within the SpokePoolClient's
deposit map, but isn't. This indirectly acts like a qualitative measure
for RPC provider responses and will allow the upper layers to
dynamically revert to more conservative behaviour.
Copy link
Member

@nicholaspai nicholaspai left a comment

Choose a reason for hiding this comment

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

This is a great solution. Do you want to add unit tests for the other InvalidDeposit codes in this PR or a follow up? I think it's ok to do a follow up since this updates all existing logic

Copy link
Contributor

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

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

LGTM!

@pxrl pxrl merged commit 1cd06de into master Jan 4, 2024
3 checks passed
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.

3 participants