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

Eip1271 Support #34

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Eip1271 Support #34

wants to merge 9 commits into from

Conversation

danfinlay
Copy link

Fixes #29

Adds Support for EIP-1271 for allowing contract accounts to give and receive delegations.

Required one additional bit to the SignedDelegation and SignedInvocation types: signerIsContract. If this bit is true, then the first 20 bytes of the signature field is treated as the address of a contract to treat as the intended signer, and that contract is sent the remaining signature: bytes along with the delegation type hash to determine for itself whether this proof should be treated as valid authorization.

I formerly was somewhat against using EIP-1271 in this way, because I had seen some contract accounts merely allow assigning a single signer as their EIP1271 recovery strategy, which completely undermines the entire point of having a contract account. My position on this has evolved a bit, to believe that correct usage is possible, and so we shouldn't let the possibility of flawed contract accounts prevent good ones from participating in this.

For example, good usage might look like a multisig might have a custom datastructure for representing which accounts' signatures are included, and including all the signatures combined in the signature payload.

Larger controller sets like a token-weighted DAO might involve too many signatures to submit as a delegation/invocation as calldata, but I'll leave exploration for how to best support that to those contracts' owners.

Fixes #29

Adds Support for [EIP-1271](https://eips.ethereum.org/EIPS/eip-1271) for allowing contract accounts to give and receive delegations.

Required one additional bit to the `SignedDelegation` and `SignedInvocation` types: `signerIsContract`. If this bit is `true`, then the first 20 bytes of the `signature` field is treated as the address of a contract to treat as the intended signer, and that contract is sent the remaining `signature: bytes` along with the delegation type hash to determine for itself whether this proof should be treated as valid authorization.

I formerly was somewhat against using EIP-1271 in this way, because I had seen some contract accounts merely allow assigning a single signer as their EIP1271 recovery strategy, which completely undermines the entire point of having a contract account. My position on this has evolved a bit, to  believe that correct usage is possible, and so we shouldn't let the possibility of flawed contract accounts prevent good ones from participating in this.

For example, good usage might look like a multisig might have a custom datastructure for representing which accounts' signatures are included, and including all the signatures combined in the `signature` payload.

Larger controller sets like a token-weighted DAO might involve too many signatures to submit as a delegation/invocation as `calldata`, but I'll leave that research as to do.
@danfinlay
Copy link
Author

Some things I might consider before merging:

  • How do we want to manage "iterations" on this repository?
  • Maybe add a git tag for the last version, so it can live on, and update all the contract references to new deploys here?
  • Should we make sure to update the docs at the same time? Just a one bit difference for backwards-compat, but also a new feature to document now.

@danfinlay
Copy link
Author

Oh this PR also fixes the timestamp-enforcer, which was mixed up. I've since learned it was being deprecated. Oh well. That's what happens when you require all tests to pass before a push :D

@gkoum-7
Copy link

gkoum-7 commented Feb 5, 2023

minor change on off chain js libs; sign invocations by calling .signInvocations() - not .signInvocation()

@danfinlay
Copy link
Author

Ah I am probably going to make another change to this. I might make that change in eip712-codegen first so I can minimize the rewriting in the future. https://twitter.com/frangio_/status/1629994287080374272?s=46&t=

@danfinlay
Copy link
Author

Ok the codegen rewrite is working, so I'm in a good position to re-implement these changes with a few others I've been wanting to make, all with auto-generated signature verification code now too:
danfinlay/eip712-codegen#11

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.

Enable delegations from contract accounts
2 participants