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

Update providerRecommendedGasPrice #1757

Merged
merged 8 commits into from
May 9, 2023
Merged

Conversation

cserb
Copy link
Contributor

@cserb cserb commented May 3, 2023

The actual price calculation for providerRecommendedGasPrice has not been tested (or I couldn't find it in any tests).
Closes: #1711

@aquarat aquarat marked this pull request as draft May 4, 2023 11:21
@cserb cserb requested a review from dcroote May 4, 2023 15:30
Copy link
Contributor

@dcroote dcroote left a comment

Choose a reason for hiding this comment

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

Nice Cris! I haven't had the opportunity to thoroughly review, but in the meantime two quick points on the PR generally (besides what you already experienced in the form of CI Prettier formatting and a changeset 😅 )

  1. For CI steps that fail, you can click on Details and rerun only the failed jobs. This is necessary occasionally, for example in your last commit where the docs CI failed for a spontaneous reason unrelated to the PR.
  2. GitHub requires specific language in order to link a PR to an issue and we leverage that link in managing the status of issues as they move across the Airnode board. For example, when a PR gets created that closes an issue, that issue will automatically be assigned the status of "In review". So instead of This implements: #1711, something like Closes #1711 (more examples here)

Also is there a link to what is referenced in the issue comment?

this is already implemented by @bdrhn9 in an Airseeker fork, it just needs to be transferred over to Airnode proper

@cserb
Copy link
Contributor Author

cserb commented May 5, 2023

Also is there a link to what is referenced in the issue comment?

this is already implemented by @bdrhn9 in an Airseeker fork, it just needs to be transferred over to Airnode proper

Unfortunately no. This was done in a private repo, that @bdrhn9 made available to me as a download. I can share it with you as well if it helps with the review.

@aquarat
Copy link
Contributor

aquarat commented May 5, 2023

@cserb The PR contents now, apart from the inclusion of tests (I assume Bedirhan's work), isn't too different to your first implementation, so well done on that. Also, it's good that you reached out to Bedirhan. Don't forget to set yourself as an assignee on the PR and switch it to "ready to review" when you're ready for review (button near bottom). The branch should also be updated ("Update branch") prior to review and eventual merging.

I was surprised that the compiler allowed you to directly compare two BigNumber objects in your first attempt and that it worked, but this version, which explicitly uses the gt function is less scary 😆

Thanks for guiding @dcroote 🙏 ❤️

@cserb cserb self-assigned this May 5, 2023
@cserb cserb marked this pull request as ready for review May 5, 2023 08:11
@cserb
Copy link
Contributor Author

cserb commented May 5, 2023

I was surprised that the compiler allowed you to directly compare two BigNumber objects in your first attempt and that it worked, but this version, which explicitly uses the gt function is less scary 😆

Honestly the first implementation was specifically to test the CI and to understand what to expect. The direct comparison was not meant to stay :)

Copy link
Contributor

@dcroote dcroote left a comment

Choose a reason for hiding this comment

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

Nice - approving with a few minor nits

dcroote

This comment was marked as duplicate.

Copy link
Contributor

@dcroote dcroote left a comment

Choose a reason for hiding this comment

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

LGTM so feel free to merge, though I'd recommend the “Squash and merge" strategy given the WIP and formatting commits.

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.

Implement the gas price strategy: Sanitized providerRecommendedGasPrice
3 participants