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 json-rpc responses for 1559 #2222

Merged
merged 12 commits into from
May 17, 2021
Merged

update json-rpc responses for 1559 #2222

merged 12 commits into from
May 17, 2021

Conversation

garyschulte
Copy link
Contributor

@garyschulte garyschulte commented May 5, 2021

Signed-off-by: garyschulte garyschulte@gmail.com

PR description

  • adds 1559-specific fields to json-rpc responses for BlockResult and TransactionResult
  • removes empty gasPrice from TransactionResult from json-rpc serialization
  • removes gasLimit from 1559 transactions json-rpc serialization
  • fixes retesteth eth_getTransactionCount to use a hex rather than decimal for result

Fixed Issue(s)

fixes #2215
fixes #2287

Changelog

@garyschulte garyschulte changed the title update json-rpc responses for 1559, use current feeCap and gasPremium… update json-rpc responses for 1559 May 5, 2021
@@ -198,6 +206,11 @@ public String getGasLimit() {
return gasLimit;
}

@JsonGetter(value = "gasTarget")
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to add tests for impacted RPC methods in the case of an eip1559 transaction ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking the same thing actually. This PR is probably best to wait until next week to merge, so we can implement whatever the consensus is from the london readiness call where json-rpc format will be discussed (hopefully finalized)

Copy link
Contributor Author

@garyschulte garyschulte May 15, 2021

Choose a reason for hiding this comment

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

I added serialization tests for the added fields, and removed gasTarget in favor of gasLimit. should be gtg

Copy link
Contributor

@matkt matkt 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 an error message to update

throw new InvalidJsonRpcParameters("gasPrice cannot be used with baseFee or feeCap");
&& (callParams.getMaxFeePerGas().isPresent()
|| callParams.getMaxPriorityFeePerGas().isPresent())) {
throw new InvalidJsonRpcParameters("gasPrice cannot be used with baseFee or maxFeePerGas");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's an old issue but we need to replace baseFee in the message (maxPriorityFeePerGas ?)

…ck config

fixes #2192

Signed-off-by: garyschulte <garyschulte@gmail.com>
…regression

Signed-off-by: garyschulte <garyschulte@gmail.com>
… field names

Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: Gary Schulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
…less

Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte garyschulte merged commit a9dcd5b into hyperledger:master May 17, 2021
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
* address assumption that if 1559 is enabled there is a london fork block config
fixes hyperledger#2192

Signed-off-by: garyschulte <garyschulte@gmail.com>

* omit null chainId from transaction json serialization, and fix merge regression

Signed-off-by: garyschulte <garyschulte@gmail.com>

* update json-rpc responses for 1559, use current feeCap and gasPremium field names

Signed-off-by: garyschulte <garyschulte@gmail.com>

* add pr number to changelog

Signed-off-by: garyschulte <garyschulte@gmail.com>

* fix transaction decoder

Signed-off-by: Gary Schulte <garyschulte@gmail.com>

* rebase and fix merge conflicts

Signed-off-by: garyschulte <garyschulte@gmail.com>

* remove null chainId from json-rpc test fixtures

Signed-off-by: garyschulte <garyschulte@gmail.com>

* the great 1559 rename

Signed-off-by: garyschulte <garyschulte@gmail.com>

* re-remove gasTarget

Signed-off-by: garyschulte <garyschulte@gmail.com>

* add serialization tests for 1559 transaction fields

Signed-off-by: garyschulte <garyschulte@gmail.com>

* add json property ordering annotations to Transaction*Result and spotless

Signed-off-by: garyschulte <garyschulte@gmail.com>

* rebase on master, update error message feedback from Karim

Signed-off-by: garyschulte <garyschulte@gmail.com>
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 JSON-RPC changes for 1559 1559 Testing RPC support
2 participants