Add name and version to bls wallet domains

JohnGuilding commented 1 year ago

What is this PR doing?

Adds name and version to BLS Wallet domains. Following offline discussion with @jzaki, we decided that we do not need to implement EIP-712 at the contract level. Instead, the EIP-712 smart contract changes are mainly relevant at the dapp/protocol level, not the contract level for smart contract wallets.

The audit did not specify directly implementing 712, but instead, using it as inspiration and additional information for completing audit issue 7 - #571:

Other recommendations are to also include a version and a name fields to allow protocol upgrades. Consider using the EIP712 domain as inspiration and additional information.

Although the contracts don't need EIP-712 support, we may want to consider adding support for EIP-712 in Quill in another issue, as it currently does not.

How can these changes be manually tested?

run tests

Does this PR resolve or contribute to any issues?

resolves #25

Checklist

[x] I have manually tested these changes
[x] Post a link to the PR in the group chat

Guidelines

If your PR is not ready, mark it as a draft
The resolve conversation button is for reviewers, not authors
- (But add a 'done' comment or similar)

jzaki commented 1 year ago

I was a little confused by the description though, I don't think I understand the comment - we decided we do not need to implement EIP-712 at the contract level. What would need to change to implement 712 at the contract level and why are we then adding a name and version into the domain?

Are these changes so that Quill can show the name and version to the user before they sign a message?

The domain does not conform to 712, as it includes "Bundle" and "Wallet" as an extra param. The name and version helps separate domains of version numbers and wallet signer name. So if the gateway contract was updated in situ (it can't be), then the version in the domain would separate messages signed for the previous version to be valid in the new version. The scope of 712 domains makes sense for domain separation at the dapp level, and for legibility as you suggest. The BLS domain is effectively at a lower level.

blakecduncan commented 1 year ago

@jzaki Nice that makes sense 👍. I was confused because I was curious what the benefit of adding a name and version if we aren't making it 712 compliant. As the only way to change the VG is to deploy a new one which would change VG address and that would change the domain. But it sounds like legibility is the main advantage here.

JohnGuilding commented 1 year ago

I was a little confused by the description though, I don't think I understand the comment - we decided we do not need to implement EIP-712 at the contract level. What would need to change to implement 712 at the contract level and why are we then adding a name and version into the domain?

@blakecduncan yeah re-reading I can see how the description is confusing. To build on what James has said, we chose to take inspiration from EIP-712 (for the reasons James mentioned), but we didn't actually implement it. This is because it isn't actually needed at the contract level for a smart contract wallet.

It would be used in a dapp smart contract as a domainSeparator, and would be specific to that smart contract. Here's an example from a uniswap contract of how it could be used. As well as being able to display such information to the user in the wallet, the domain in this case is used to ensure that signed typed data cannot be confused between different protocols/dapps. E.g. there's a possibility different dex's may have the same signed message struct. So the domain separator helps differentiate between such transactions and prevents signature collision.

blakecduncan commented 1 year ago

I was a little confused by the description though, I don't think I understand the comment - we decided we do not need to implement EIP-712 at the contract level. What would need to change to implement 712 at the contract level and why are we then adding a name and version into the domain?

@blakecduncan yeah re-reading I can see how the description is confusing. To build on what James has said, we chose to take inspiration from EIP-712 (for the reasons James mentioned), but we didn't actually implement it. This is because it isn't actually needed at the contract level for a smart contract wallet.

It would be used in a dapp smart contract as a domainSeparator, and would be specific to that smart contract. Here's an example from a uniswap contract of how it could be used. As well as being able to display such information to the user in the wallet, the domain in this case is used to ensure that signed typed data cannot be confused between different protocols/dapps. E.g. there's a possibility different dex's may have the same signed message struct. So the domain separator helps differentiate between such transactions and prevents signature collision.

@JohnGuilding Yeah that makes sense. I think my main point is that we already have a domain separator, it just isn't in the EIP-712 format.

When I added the domains to address audit issue 7 I decided not to include a name and version because it wasn't needed to address the audit issue and it was added scope to fully implement eip712. So I think to rephrase my original question, I was curious what the benefit of adding a name and version is if we aren't doing it for EIP-712 compatibility (since we already prevent signature collision). It sounds like we do get legibility benefits for dApps though

JohnGuilding commented 1 year ago

@blakecduncan ok gotcha, I see where your question was coming from now 👍

getwax / bls-wallet