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

Re-implement "Estimate gas for RRP fulfillments" #1817

Merged
merged 9 commits into from
Jul 10, 2023

Conversation

bdrhn9
Copy link
Contributor

@bdrhn9 bdrhn9 commented Jun 26, 2023

Closes #1589

This PR basically:

  • Adds back Estimate gas for RRP fulfillments which was reverted and reason had been nicely told in the Slack thread
  • Refactors validator to check, validate, populate default address for AirnodeRrpV0DryRun contract
  • Separates estimation of RRP overhead and external call (fulfillment call) overhead (AirnodeRrpV0DryRun is being leveraged to estimate overhead for RRP protocol)
  • Fixes incorrect chainId where deployments were absent (although this issue allowed the tests to pass, it resulted in an undefined-to-undefined equality check)
  • Fixes typo in comment

I think it'll be better to refer the provided Slack thread first. Btw I'm not sure about PR title created by GitHub when reverted the ancestor PR, let me know if it needs to be changed.

Sorry for force pushs, I would like to stick with @dcroote 's commit message protocol :-)

@bdrhn9 bdrhn9 self-assigned this Jun 26, 2023
@bdrhn9 bdrhn9 force-pushed the revert-1710-revert-1693-estimate-gas-for-fulfillments branch from b7adb2f to 2b273b0 Compare July 5, 2023 23:25
@bdrhn9 bdrhn9 force-pushed the revert-1710-revert-1693-estimate-gas-for-fulfillments branch from 2b273b0 to 93bc917 Compare July 6, 2023 08:25
@bdrhn9 bdrhn9 marked this pull request as ready for review July 6, 2023 09:32
@bdrhn9 bdrhn9 requested a review from dcroote July 6, 2023 11:07
@@ -62,7 +64,8 @@ export const chainTypeSchema = z.literal('evm');

export const airnodeRrpContractSchema = z
.object({
AirnodeRrp: evmAddressSchema,
AirnodeRrp: evmAddressSchema.optional(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is a clarification to make it clearer: Although AirnodeRrp shouldn't be optional, I have set both contracts to be optional in this case. The reason behind this decision is to accommodate situations where the user wants to use their own address for one of the contracts, while using default deployments for the other contract. In such cases, the user can define chains.contracts for the desired contract but leave it undefined for the contract they wish to use default deployments for.

Actually this schema is input schema (see here) and supposed to parse input config.json. Enforcing this schema with transforms below, inferred output type for this schema will be {AirnodeRrp: evmAddressSchema, AirnodeRrpDryRun: evmAddressSchema.optional()}

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 work and I have mostly minor comments. For the PR title, perhaps something like "Re-implement estimate gas for RRP fulfillments"?

I was hoping there was going to be a simple way to incorporate this into an E2E test, but I don't see one at the moment. The airnode-examples E2E test might be a good candidate, but it would involve hardhat contract deployment similar to AirnodeRrp e.g. after this line.

@bdrhn9 bdrhn9 changed the title Revert "Revert "Estimate gas for RRP fulfillments"" Re-implement "Estimate gas for RRP fulfillments" Jul 9, 2023
@bdrhn9 bdrhn9 merged commit cf1d73e into master Jul 10, 2023
@bdrhn9 bdrhn9 deleted the revert-1710-revert-1693-estimate-gas-for-fulfillments branch July 10, 2023 08:37
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.

Have RRP fulfillments estimate gas
2 participants