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 new _gasPrice reserved parameter #1609

Merged
merged 6 commits into from
Jan 15, 2023
Merged

Conversation

dcroote
Copy link
Contributor

@dcroote dcroote commented Jan 11, 2023

Allows gasPrice to be specified by a requester.

Closes #1492

Allows gasPrice to be specified by a requester
@dcroote dcroote force-pushed the dcroote/gasPriceReserved branch from f14f363 to 28b5436 Compare January 11, 2023 07:03
@dcroote dcroote self-assigned this Jan 11, 2023
packages/airnode-adapter/src/types.ts Show resolved Hide resolved
packages/airnode-node/src/types.ts Outdated Show resolved Hide resolved
packages/airnode-node/src/api/index.ts Show resolved Hide resolved
packages/airnode-node/src/types.ts Outdated Show resolved Hide resolved
@dcroote dcroote requested a review from a team January 11, 2023 07:25
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.

LGTM, just a minor note. I want to test it first before approving.

packages/airnode-node/src/types.ts Outdated Show resolved Hide resolved
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.

Have you tested this? Because I did and I'm pretty sure it couldn't have worked 😄
It took me quite some time but I found out what the problem is and why the value is ignored. The problem is here
https://github.com/api3dao/airnode/blob/master/packages/airnode-node/src/coordinator/calls/disaggregation.ts#L41
The response there is reconstructed but only the fields data and success are added. That was enough before but now we need to also add the reservedParameterOverrides field.
TS didn't complain because the field is optional anyway. But the set _gasPrice value was ignored.

Also, even though the value correctly overrides what's returned from the gas price oracle, the logs are quite confusing. They say how is the value chosen by the oracle but they don't mention anything about it being overridden later.

I'd also suggest writing tests using these reserved parameter overrides so that we won't run into something similar again.

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.

LGTM

@dcroote
Copy link
Contributor Author

dcroote commented Jan 15, 2023

Oof 🤦 😞 well an apology is in order @amarthadan. I thought I had covered it through the tests I updated, but clearly you were right in this not working due to reconstruction. I added the E2E and confirmed it's working as expected locally. Sorry you ended up spending time debugging.

@dcroote dcroote merged commit 620aa0e into master Jan 15, 2023
@dcroote dcroote deleted the dcroote/gasPriceReserved branch January 15, 2023 21:03
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 gasPrice to be specified
2 participants