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 EIP-2681: Minor clarification #5434

Closed
wants to merge 3 commits into from

Conversation

xinbenlv
Copy link
Contributor

@xinbenlv xinbenlv commented Aug 9, 2022

@axic: I was minorly confused by the wording so I propose some clarification..

Change from

Limit account nonce to be between `0` and `2^64-1`.

to Option 1

Limit account nonce to be between `0` and `2^64-1`(inclusive).

or Option 2

Limit account nonce to be `0<=account_nonce<=2^64-1`

to clarify in the summary the inclusivity of two ends

@axic: I was minorly confused by the wording so I propose some clarification..
Change from 
```
Limit account nonce to be between `0` and `2^64-1`.
```
to Option 1
```
Limit account nonce to be between `0` and `2^64-1`(inclusive).
```
or Option 2
```
Limit account nonce to be `0<=account_nonce<=2^64-1`
```
to clarify in the summary the inclusivity of two ends
@eth-bot eth-bot enabled auto-merge (squash) August 9, 2022 14:00
@xinbenlv xinbenlv changed the title Minor clarification Minor clarification for EIP-2681 (final) Aug 9, 2022
@eth-bot
Copy link
Collaborator

eth-bot commented Aug 9, 2022

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):


(fail) eip-2681.md

classification
updateEIP
  • eip-2681.md requires approval from one of (@axic)

@github-actions
Copy link

github-actions bot commented Aug 9, 2022

The commit 005fe3b (as a parent of 1232e53) contains errors. Please inspect the Run Summary for details.

Pandapip1
Pandapip1 previously approved these changes Aug 11, 2022
Copy link
Member

@Pandapip1 Pandapip1 left a comment

Choose a reason for hiding this comment

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

I, personally, am fine with this change. It only changes the abstract and introduces a useful clarification.

@xinbenlv
Copy link
Contributor Author

Thank you. Since there is no concern on the editor side, I will leave it to the author to make a choice of the 3 options or leave it as-is: @axic

auto-merge was automatically disabled August 21, 2022 18:02

Head branch was pushed to by a user without write access

@xinbenlv xinbenlv requested a review from eth-bot as a code owner August 21, 2022 18:02
Pandapip1
Pandapip1 previously approved these changes Aug 22, 2022
@Pandapip1 Pandapip1 added a-review Waiting on author to review and removed classification: update EIP labels Aug 26, 2022
@Pandapip1 Pandapip1 changed the title Minor clarification for EIP-2681 (final) Update EIP-2681: Minor clarification Aug 28, 2022
@github-actions
Copy link

There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

@github-actions github-actions bot added w-stale Waiting on activity and removed w-stale Waiting on activity labels Sep 14, 2022
@xinbenlv
Copy link
Contributor Author

Friendly ping author @axic

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.

I prefer option 2 over option 1, but generally don't think a change is necessary here.

@xinbenlv
Copy link
Contributor Author

xinbenlv commented Oct 3, 2022

Thanks @lightclient. I like option 2 better too.

I was personally confused by original wording hence the PR. Let's see if @axic think needed? I am ok either way.

@xinbenlv
Copy link
Contributor Author

@axic friendly ping for author approval. Or let me know if this PR is useful or shall be dropped

EIPS/eip-2681.md Outdated
@@ -1,6 +1,7 @@
---
eip: 2681
title: Limit account nonce to 2^64-1
description: Limit account nonce to be between `0` and `2^64-1`(inclusive).
Copy link
Member

Choose a reason for hiding this comment

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

The inclusive wording is materially incorrect.

The specification states:

  1. Consider any transaction invalid, where the nonce exceeds or equals to 2^64-1.

Copy link
Contributor Author

@xinbenlv xinbenlv Nov 11, 2022

Choose a reason for hiding this comment

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

Oh, yes you are right. (embarrassed... I was such an idiot T.T ....)

Updated to 0<=account_nonce<2^64-1.

Please feel free to reject this PR and I will withdraw if you think this is not necessary. @axic

@axic
Copy link
Member

axic commented Nov 10, 2022

The bot states only:

eip-2681.md requires approval from one of (@axic)

I wonder if it is not displaying the warning anymore that this is modifying a Final EIP?

@github-actions
Copy link

github-actions bot commented Dec 3, 2022

There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

@github-actions github-actions bot added the w-stale Waiting on activity label Dec 3, 2022
@github-actions
Copy link

This pull request was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.

@github-actions github-actions bot closed this Jan 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-review Waiting on author to review c-update Modifies an existing proposal s-final This EIP is Final t-core w-stale Waiting on activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants