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-5267: Move to Last Call #6300

Merged
merged 2 commits into from Jan 10, 2023

Conversation

frangio
Copy link
Contributor

@frangio frangio commented Jan 8, 2023

No description provided.

@frangio frangio requested a review from eth-bot as a code owner January 8, 2023 18:25
@github-actions github-actions bot added c-status Changes a proposal's status t-erc labels Jan 8, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented Jan 8, 2023

All tests passed; auto-merging...

(pass) eip-5267.md

classification
updateEIP
  • passed!

@github-actions
Copy link

github-actions bot commented Jan 8, 2023

The commit 9060947 (as a parent of c693f67) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot added the w-ci Waiting on CI to pass label Jan 8, 2023
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Jan 8, 2023
@Pandapip1 Pandapip1 changed the title EIP-5267: Move to Last Call Update EIP-5267: Move to Last Call Jan 9, 2023
@xinbenlv
Copy link
Contributor

xinbenlv commented Jan 10, 2023

Congrats on moving to last call. This is an long awaited improvement~

Cross posting some editorial questions hoping to get @frangio 's clarification I left on FEM


Discussion: can you share the rationale of extensions being format as uint256 instead of bytes32 ?

Extensions are described by their EIP numbers because EIP-712 states: “Future extensions to this standard can add new fields […] new fields should be proposed through the EIP process.”

Here EIP-712 didn't specify what format of fields will be, so it's open to future designer to design, EIP-5267 is one of such future design.

Q1. Could author share your thoughts into why choose the format of extensions an uint256[] vs bytes32[] or generally bytes? This is one of the problem that EIP-5750 is trying to address and thus when EIP-5267 made a design choice that's different, I'd love to learn the rationale and understand it better. When using uint256[] and describe in EIP-5267 as it needs to be a EIP number, does it mean only EIP number will be allowed in the extension as a way to provide extending information? Will EIP number be sufficient? Do you anticipate some of these elements could be re-purposed in the future?

Q2. In the reference implementation,

    return (
          hex"0d", // 01101
          "Example",
          "",
          block.chainid,
          address(this),
          bytes32(0),
          new uint256[](0)
      );

and also in javascript

  if (extensions.length > 0) {
    throw Error("Extensions not implemented");
  }

Is it intentional that in author's perspective the extension shall at least have one element?

@frangio
Copy link
Contributor Author

frangio commented Jan 10, 2023

Is it intentional that in author's perspective the extension shall at least have one element?

No. This is clear in the specification. There are no constraints on the length of the extensions array. Additionally the Solidity reference implementation returns an empty array.

Will reply to the other questions in the discussions thread as I don't think they are editorial.

@Pandapip1 Pandapip1 closed this Jan 10, 2023
@Pandapip1 Pandapip1 reopened this Jan 10, 2023
@eth-bot eth-bot enabled auto-merge (squash) January 10, 2023 18:52
@eth-bot eth-bot merged commit c550c57 into ethereum:master Jan 10, 2023
@xinbenlv
Copy link
Contributor

xinbenlv commented Jan 10, 2023

Will reply to the other questions in the discussions thread as I don't think they are editorial.

Sure. Some questions are in editorial nature (meaning non-technical) e.g. we suggest spell out / clarify some rationale in some design choices that the author made. But those editorial questions are not merge-blocking and can be addressed in the followup PRs.

@frangio frangio deleted the 5267-backwardscompat branch January 10, 2023 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-status Changes a proposal's status t-erc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants