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

Add handling of _minConfirmations reserved parameter #1615

Merged
merged 17 commits into from
Feb 2, 2023
Merged

Conversation

dcroote
Copy link
Contributor

@dcroote dcroote commented Jan 20, 2023

Closes #1393

  • Fetch all requests assuming a minConfirmations of 0 if _minConfirmations reserved parameter set (see comment below)
  • Parse _minConfirmations from request
  • Reject requests with _minConfirmations larger than BLOCK_COUNT_HISTORY_LIMIT
  • Filter requests according to minConfirmations (either from chains[n].minConfirmations or, if allowed and present, _minConfirmations request parameter
  • Ignore _minConfirmations passed as a request parameter if _minConfirmations reserved parameter is not present for an endpoint
  • Working e2e test
  • Refactor such that minConfirmations filtering occurs before API calls are made
  • Handle users submitting requests with different _minConfirmations - requests are filtered according to the reserved parameter in the specific request, then those remaining are assigned nonces as before. Per the original spec, the max _minConfirmations for each sponsor is applied to all requests in the queue

@dcroote dcroote marked this pull request as ready for review January 26, 2023 07:40
@dcroote dcroote requested a review from a team January 26, 2023 07:41
@dcroote dcroote requested review from amarthadan and removed request for amarthadan January 31, 2023 09:19
Copy link
Contributor

@amarthadan amarthadan left a comment

Choose a reason for hiding this comment

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

Looks good, mostly nits. But please try reworking the tests so you don't need to change the startCoordinator interface.
I'm approving to move it along but I will test it later as well.

packages/airnode-node/src/providers/state.ts Outdated Show resolved Hide resolved
packages/airnode-node/src/handlers/start-coordinator.ts Outdated Show resolved Hide resolved
packages/airnode-node/src/handlers/start-coordinator.ts Outdated Show resolved Hide resolved
packages/airnode-node/src/workers/local-handlers.ts Outdated Show resolved Hide resolved
@amarthadan
Copy link
Contributor

I tested the new feature and it looks like it's working correctly, nicely done 🙂

@dcroote dcroote merged commit dafe972 into master Feb 2, 2023
@dcroote dcroote deleted the dcroote/issue1393 branch February 2, 2023 07:36
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.

Add a new reserved parameter to allow minConfirmations to be specified
2 participants