-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Add EIP-6077: Invalidation for Signature-Based Operations #6077
Conversation
Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s): (fail) eip-6077.md
|
In the context of permits I think I've seen you suggest that nonces shouldn't be sequential, so that there could be several valid permits created "in parallel". What's your current thinking on that? |
@frangio maybe we can rename |
Can you name use cases that require parallel nonces? Is this common or a good idea? How does a client know which nonce to use? I'm trying to understand what extra complexity it would require and whether it might be worth it. |
@frangio I discovered these 5 operations in OpenZeppelin/openzeppelin-contracts:
|
Based on these examples I don't think parallel nonces are needed. |
@frangio I see we probably don’t need parallel operation ids. But maybe we don’t have good invalidation technique, which can be invented later. I suspect there could be some cases where operation id contains spender/delegatee address and rest 96 bits are being used as nonce. |
I'd remove I think if there is parallelism it will still be sequential but the sequence will depend on more parameters, not just the signer. This could be expressed in the interface by including an additional argument like |
@frangio what about having definition for thread-id? Original sequential behaviour have thread-id = signer, but we could have thread-id = hash(signer, spender). Nonces will still be sequential but one signer could have multiple threads of ids... |
@Pandapip1 could you help us here with EIP number? |
@k06a A thread id would work but it's similar to (though more efficient than) IMO, the ideal end goal for full generality would be the ability to create a function like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the walidator errors
Co-authored-by: Pandapip1 <[email protected]>
Co-authored-by: Pandapip1 <[email protected]>
Co-authored-by: Pandapip1 <[email protected]>
Co-authored-by: Pandapip1 <[email protected]>
Co-authored-by: Sam Wilson <[email protected]>
|
||
Same abstraction could be utilized by mutilple signature-based operations, moreover existing EIPs like [EIP-2612](./eip-2612.md) could be considered as fully compatible with the EIP. | ||
|
||
Multiple EIPs define operations that can be executed in behalf of signer and sometime introduce method naming collisions and other. For example, [EIP-2612](./eip-2612.md) defines both `permit` and `nonces` methods, but gives no clue that `nonces` is related to permit operation. In case of multiple same-level EIPs implemented within one smart contract (for example: permit, delegate, vote) it's obvious that they should use different nonces. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't understand what you're trying to say here. Mind explaining?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to create abstraction for signature-based operations, and this abstraction also covers existing EIP-2612 somehow.
|
||
## Motivation | ||
|
||
Same abstraction could be utilized by mutilple signature-based operations, moreover existing EIPs like [EIP-2612](./eip-2612.md) could be considered as fully compatible with the EIP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reads like Rationale, not Motivation. Mind moving this to that section?
- main sequnce of incremental nonces per signer | ||
- sequence of incremental ids per signer per beneficiary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be complete sentences
} | ||
``` | ||
|
||
- Operation EIPs SHOULD use at leat one of the nonces or ids sequences per signer defined by this EIP or both. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not make this a MUST? Also, the phrasing here is a bit odd. Mind explaining?
Co-authored-by: Pandapip1 <[email protected]>
Co-authored-by: Pandapip1 <[email protected]>
The commit 911c35b (as a parent of b74e02d) contains errors. |
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. |
Don't close it, it's still under discussion and consideration |
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. |
@frangio what do you think? Is it worth fixing? Do you still consider this kind of functionality in OZ library? |
@k06a We're not quite thinking about this at the moment. Personally I have to look into this topic a bit more and it may need more research and discussion, and I don't think this is the place for that, so I would close this and might revisit later. |
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. |
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. |
No description provided.