EIPs icon indicating copy to clipboard operation
EIPs copied to clipboard

Update EIP-4494: Include EIP-1271 and EIP-2098 as required

Open TimDaub opened this issue 2 years ago • 12 comments

EIP-1271:

  • EIP-4494 currently makes no mention of EIP-1271 but I strongly assume that there is interest to support signature validation through a smart contract
  • There are strong reasons to believe so as @dievardump's reference implementation uses OZ's SignatureChecker that in turn implements EIP-1271
  • Unclear to me: Whether or not the authors of the EIP document want to further mention 1271. Anyways, at least I think it should be mentioned in the header as required.

EIP-2098:

  • document mentions: "sig is a valid secp256k1 or EIP-2098 signature from owner of the tokenId:"

TimDaub avatar Jun 24 '22 06:06 TimDaub

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


(fail) eip-4494.md

classification
updateEIP
  • eip-4494.md requires approval from one of (@dievardump, @wschwab)

eth-bot avatar Jun 24 '22 06:06 eth-bot

@MicahZoltu @lightclient could one of you manually merge this?

Pandapip1 avatar Jul 15 '22 17:07 Pandapip1

We need to get approval from one of the authors before merging. The author may prefer to remove the reference to 2098 to resolve that one, and they may not want to require 1271 for some reason. We should not let 4494 go to review without resolving the 2098 issue though.

MicahZoltu avatar Jul 16 '22 11:07 MicahZoltu

@dievardump @wschwab ☝🏼

TimDaub avatar Jul 25 '22 19:07 TimDaub

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[bot] avatar Sep 17 '22 00:09 github-actions[bot]

I don't see why 1271 or 2098 should be required. We have designed the EIP to be compatible with them, but at no point in the implementation are they required. Feel free to change my mind.

wschwab avatar Sep 17 '22 18:09 wschwab

Feel free to change my mind.

It should be explicitly stated that they may be supported, then, because EIP-1271 is assumed to not be supported.

Pandapip1 avatar Sep 19 '22 00:09 Pandapip1

I would prefer not to add those as required, because I think the implementers should be able to decide which ones they want to support. Including possible future signature standards (which is one of the reasons why we used bytes instead of a decomposed form)

So we should probably remove the eip-2098 mentions and make that more generic if this is a problem and is what leads to assume that "EIP-1271 to not be supported."


As a reference, "EIP-2098" is mentioned 3 times

/// @param sig a traditional or EIP-2098 signature

sig is a valid secp256k1 or EIP-2098 signature from owner of the tokenId:

in order to support EIP-2098 signatures (64 bytes)

dievardump avatar Sep 19 '22 12:09 dievardump

I don't see why 1271

fair.

2098 should be required

You mention EIP-2098 as also @dievardump points out, and it is how you format the signature as bytes, and in that case, IMO, it should be added to the requirements.

TimDaub avatar Sep 19 '22 14:09 TimDaub

we point it out, but we never say that a 4494-compliant implementation MUST support either, imho a requirement should be something required, not something that was merely accommodated

wschwab avatar Sep 19 '22 16:09 wschwab

we point it out, but we never say that a 4494-compliant implementation MUST support either, imho a requirement should be something required, not something that was merely accommodated

fine. defining it like this can be a pro and a con.

Pro: if a better compact signature scheme arrives it may be usable

con: being loose can lead to undefined behavior.

I tend to prefer being very explicit and specific but it's ur guy's call, feel free to close

TimDaub avatar Sep 20 '22 09:09 TimDaub

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[bot] avatar Oct 17 '22 00:10 github-actions[bot]

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[bot] avatar Nov 29 '22 00:11 github-actions[bot]