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

accounts: eip-712 signing for ledger #22378

Merged
merged 2 commits into from
Mar 22, 2021
Merged

Conversation

MrChico
Copy link
Member

@MrChico MrChico commented Feb 24, 2021

Adds a new method SignTypedMessage to the accounts.Wallet type which enables EIP-712 signing with ledgers. For other wallet types it simply falls back to signHash.
Support for 712 style signatures was introduced in v1.5.0 of the ledger firmware, and is specified at https://github.com/LedgerHQ/app-ethereum/blob/master/doc/ethapp.asc#sign-eth-eip-712.
This PR does not expose the SignTypedMessage to clef or anywhere else, I mainly wanted it in ethsign which is really just a cli wrapper around these parts of the geth code.

I don't really know golang so bare with me here... At least I'm able to successfully perform EIP-712 signatures with a ledger with this PR.

accounts/scwallet/wallet.go Outdated Show resolved Hide resolved
@MrChico MrChico force-pushed the eip712ledger branch 2 times, most recently from a830a23 to e173a5d Compare February 25, 2021 12:45
@holiman
Copy link
Contributor

holiman commented Feb 25, 2021

I don't know if another API-method is really needed. Clef already supports this, by using the mimetype in SignData to signal that it's 712-type data that's being passed.
So the same mechanism can be used by other backends too.

@MrChico
Copy link
Member Author

MrChico commented Feb 25, 2021

The problem is that ledgers won't sign arbitrary data, it will only take the two 32-byte strings messageHash and domainHash. But maybe these could be extracted for the case where the special case where mimetype is typed data?

@holiman
Copy link
Contributor

holiman commented Feb 25, 2021

Isn't that more or less what clef does too:

rawData := []byte(fmt.Sprintf("\x19\x01%s%s", string(domainSeparator), string(typedDataHash)))
?

@MrChico
Copy link
Member Author

MrChico commented Feb 25, 2021

right, only it would be deconstructing that information, rather than building it.
so I'm thinking to add a special case to SignData of usbwallet/wallet.go which dispatches to driver.SignTypedMessage in the case where

if mimeType == accounts.MimetypeTypedData && data[0] == 25 && data[1] == 1 && len(data) == 66 {

@MrChico MrChico force-pushed the eip712ledger branch 2 times, most recently from a2c4368 to cf90d37 Compare February 25, 2021 14:21
@MrChico
Copy link
Member Author

MrChico commented Feb 25, 2021

somthing like this @holiman , what do you think?

accounts/usbwallet/ledger.go Outdated Show resolved Hide resolved
accounts/usbwallet/wallet.go Outdated Show resolved Hide resolved
accounts/usbwallet/wallet.go Outdated Show resolved Hide resolved
return nil, err
}
return signature, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole clause is more or less a copy-paste of SignTx. Is there any way it can be refactored so they both reuse the whole dance around commsLock and account-lookup and stuff, and not have the same code duplicated?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm not sure exactly how to do this, since the defered functions are relative to the scope of the surrounding function. Do you have an idea?

accounts/usbwallet/wallet.go Outdated Show resolved Hide resolved
accounts/usbwallet/wallet.go Outdated Show resolved Hide resolved
@MrChico
Copy link
Member Author

MrChico commented Mar 1, 2021

Thank you @holiman for the review. Besides the refactoring of the commsLock dance, your comments should now be addressed

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@holiman holiman merged commit aab3560 into ethereum:master Mar 22, 2021
atif-konasl pushed a commit to frozeman/pandora-execution-engine that referenced this pull request Oct 15, 2021
* accounts: eip-712 signing for ledger

* address review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants