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

1559 JSON RPC Diff #47

Merged
merged 9 commits into from
May 19, 2021
Merged

1559 JSON RPC Diff #47

merged 9 commits into from
May 19, 2021

Conversation

alita-moore
Copy link
Contributor

@alita-moore alita-moore commented May 11, 2021

(sorry about the bad commit name)
This..

  • adds baseFee to the Block type
  • removes gasPrice from Transaction type
  • adds maxPriorityFeePerGas to Transaction type
  • adds maxFeePerGas to Transaction type

"baseFee": {
"title": "baseFee",
"description": "The block's baseline network fee per gas; the baseFee is burned",
"$ref": "#/components/schemas/Integer"
Copy link
Contributor

@MicahZoltu MicahZoltu May 11, 2021

Choose a reason for hiding this comment

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

Is there a type for UnsignedInteger256? This appears to be UnsignedInteger but is not constrained to 256-bits (implying that a 512-bit number is an acceptable value).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, I can add that. Although goes a little beyond the scope of 1559 changes. As I understand it this was copy pasted from ETC so I'll be doing some conditioning like you mention soon

json-rpc/spec.json Outdated Show resolved Hide resolved
Copy link
Contributor

@MicahZoltu MicahZoltu left a comment

Choose a reason for hiding this comment

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

We need to make it clear that it is not valid to have both gasPrice and maxPriorityFeePerGas and other similar combinations.

@timbeiko
Copy link
Contributor

timbeiko commented May 11, 2021

Thanks for doing this @alita-moore! We agreed to expose the EIPs' concepts with the following terms in #allcoredevs yesterday:
* baseFee -> baseFeePerGas
* maxPriorityFeePerGas -> priorityFeePerGas
* maxFeePerGas -> feeCapPerGas

Could you edit to use the updated terms? Also, perhaps we should explain in the description of each term which "spec term" it corresponds to. WDYT?

Thank you!

Edit: not relevant anymore, see #47 (comment)

json-rpc/spec.json Outdated Show resolved Hide resolved
@timbeiko
Copy link
Contributor

@alita-moore sorry for the back and forth but as per this it seems there is still contention w.r.t. the specific naming. Let's wait and see before we make more changes. Sorry about the back and forth!

@timbeiko
Copy link
Contributor

@alita-moore FYI we agreed on ACD this AM to use the terms from the EIP, so baseFeePerGas, maxFeePerGas, maxPriorityFeePerGas.

json-rpc/spec.json Outdated Show resolved Hide resolved
Copy link
Contributor

@MicahZoltu MicahZoltu left a comment

Choose a reason for hiding this comment

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

Would you mind splitting the spec changes from the spec rename? Sadly, GitHub doesn't seem to recognize this is a file rename so we can't see the "diff", which is valuable from a history standpoint.

I recommend doing the file move first, so that change history is easily traversed.

@holgerd77
Copy link
Contributor

What is actually the reason for this renaming respectively is this really necessary? Will there be more specs in different formats in the future?

In case renaming is wanted short note that there is a link in the main README.md which then would also need an update.

@lightclient
Copy link
Member

I agree, I don't really see a reason to change until there are more spec formats.

@alita-moore
Copy link
Contributor Author

@holgerd77 the reason is because the OPEN-RPC vscode plugin uses the openrpc.json as a way to find the files to parse. It's not required of course, but it is helpful when making changes. Either way I have reverted the change

@timbeiko
Copy link
Contributor

@alita-moore @lightclient @MicahZoltu is this OK to merge now?

@alita-moore
Copy link
Contributor Author

@MicahZoltu just a heads I'm going to address your comment about the integer in a separate PR

@MicahZoltu
Copy link
Contributor

If we are going to rename this (and I think @alita-moore's argument for renaming is quite reasonable) we should do it before this PR so history doesn't get confused by the rename.

@alita-moore
Copy link
Contributor Author

@MicahZoltu i will be making significant changes to the structure of the spec. I thinks it's best to just do it after. There is no practical difference in my opinion

json-rpc/spec.json Outdated Show resolved Hide resolved
Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

looks correct to me

json-rpc/spec.json Outdated Show resolved Hide resolved
Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

Just caught an issue actually.

timbeiko and others added 2 commits May 19, 2021 08:58
Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
@timbeiko timbeiko merged commit a813bf6 into master May 19, 2021
lightclient added a commit to lightclient/execution-specs that referenced this pull request Jun 30, 2021
Add upstream links to sphinx templates
@ethereum ethereum deleted a comment Feb 13, 2023
@SamWilsn SamWilsn deleted the 1559-spec-diff branch April 10, 2024 23:52
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.

5 participants