-
Notifications
You must be signed in to change notification settings - Fork 72
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: enable gas estimation in airnode-examples local flow and E2E test #1836
Conversation
@@ -181,7 +181,7 @@ const createConfig = async (generateExampleFile: boolean): Promise<Config> => ({ | |||
|
|||
const generateConfig = async (generateExampleFile = false) => { | |||
const config = await createConfig(generateExampleFile); | |||
generateConfigFile(__dirname, config, generateExampleFile); | |||
await generateConfigFile(__dirname, config, generateExampleFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think await
here is redundant because after this line function is ending.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially thought so too and spent a lot of time dealing with strange behavior. Turns out you do need it. I created a simple example to demonstrate. If you run it, "All done" will be printed before the (non-awaited) async call completes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, thanks much for this self-explanatory example. It makes me understand the behavior. 🙏🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the implementation, it's good idea to incorporate estimating gas feature with airnode-examples.
Just for different perspective, I'd prefer to treat AirnodeRrpV0DryRun contract like we do for AirnodeRrpV0. So:
- Firstly I'd implement the function similar to getExistingAirnodeRrpV0
- Then I'd populate deployed AirnodeRrpV0DryRun address to .json file under
packages/airnode-examples/deployments
as in here by utilizing the functiongetExistingAirnodeRrpV0DryRun
in first step - I'd create the function similar to getAirnodeRrpAddress
- Then, I'll use the function
getAirnodeRrpDryRunAddress
here to populatechains[*].contracts.AirnodeRrpDryRun
@bdrhn9 - I very much appreciate your thoughts and code links, but I'd actually like to do the exact opposite 😅 What I mean is that I implemented the functions you're referring to before we defaulted to the API3 deployed AirnodeRrpV0 contract in Airnode. Now that we have that functionality I'd like to remove and refactor this |
Closes #1692
Closes #1833
PR title mostly speaks for itself, with the key phrase being "local flow", see the description in #1833 for why.
I thought it best to solve both of these issues together rather than just hack a solution for the E2E test. The CI logs demonstrate that gas is being estimated (and therefore the contract is deployed) successfully: