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

Introduce SequentialOperations abstraction for signature-based ops #3850

Closed

Conversation

k06a
Copy link
Contributor

@k06a k06a commented Dec 2, 2022

Beware that this PR is build on top of #3848 and includes its commits

PR Checklist

TBD

  • Tests
  • Documentation
  • Changelog entry

@k06a k06a force-pushed the feature/operations-abstraction branch from 57aab49 to c02c77c Compare December 2, 2022 16:47
@k06a
Copy link
Contributor Author

k06a commented Dec 2, 2022

Abstraction related to this implementation is also discussed here: ethereum/EIPs#6077

@k06a k06a force-pushed the feature/operations-abstraction branch 3 times, most recently from af72e52 to 31e96f0 Compare December 2, 2022 17:07
…and signature-based operations like ERC20Permit, Votes, Governor, MinimalForwarder
@k06a k06a force-pushed the feature/operations-abstraction branch from 31e96f0 to 4f3422c Compare December 2, 2022 17:24
… Governor, since it is relying on child contract to invalidate by proposalId and voter.
@k06a k06a changed the title Introduce SequentialOperations and ParallelOperations Introduce SequentialOperations abstraction for signature-based ops Dec 2, 2022
@Amxx
Copy link
Collaborator

Amxx commented Jan 4, 2023

Hello @k06a

This looks like a very significant change. You are rightfully targeting next-5.0 as this would be breaking. Still, I believe we should have more discussion before jumping to an implementation.

I appreciate that there is standardization effort making an ERC with that, but it is still in a very early stage, and likely to evolve. Maybe a good option would be to keep the current interface/approach, and "just" change the storage under the hood to be future proof and allow this to be implemented as a non-storage-breaking change during 5.x lifecycle.

There are other nonce related changes that we could implement. I'm thinking of the timeline system described here.

Lets discuss features first. Don't worry, the 5.0 train is not leaving anytime soon :)

@Amxx
Copy link
Collaborator

Amxx commented Jan 4, 2023

Note: we might reopen this based on the discussion in #3927

@TrejGun
Copy link

TrejGun commented Jan 5, 2023

@k06a please explain where operationHash comes from?

@k06a
Copy link
Contributor Author

k06a commented Jan 5, 2023

@TrejGun it depends on specific operation EIP/definition.

@TrejGun
Copy link

TrejGun commented Jan 5, 2023

@k06a can you show any example?

@k06a
Copy link
Contributor Author

k06a commented Jan 5, 2023

@TrejGun my bad, you should use EIP-712 for structured data hash.

@TrejGun
Copy link

TrejGun commented Jan 6, 2023

@k06a
Ok I will ask again)

did you abstract this code as operationHash?

       keccak256(
          abi.encode(
            SOME_TYPEHASH,
            same_variable1,
            same_variable2,
            ...
          )
        )

I ask this because I really like this abstraction and it does not look like you are doing this in solidity so maybe in JS and I want to understand how. For now I use ethers._signTypedData that will return _hashTypedDataV4(operationHash) but not just operationHash. So how can I generate it?

looks like ethers.utils.AbiCoder.encode(["uint256"], [1]); but it also looks like a big pain for complex types

@k06a
Copy link
Contributor Author

k06a commented Jan 8, 2023

@TrejGun
Copy link

TrejGun commented Jan 9, 2023

@k06a thanks

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.

None yet

3 participants