-
Notifications
You must be signed in to change notification settings - Fork 48
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
feat: add support for EIP712 and eth address standard #2
Conversation
61b257f
to
b3e2193
Compare
fe51c57
to
bbf884a
Compare
bbf884a
to
dc4aec2
Compare
app/ante/eip712.go
Outdated
// VerifySignature of ethsecp256k1 accepts 64 byte signature [R||S] | ||
// WARNING! Under NO CIRCUMSTANCES try to use pubKey.VerifySignature there | ||
if !secp256k1.VerifySignature(pubKey.Bytes(), sigHash, feePayerSig[:len(feePayerSig)-1]) { | ||
return sdkerrors.Wrap(sdkerrors.ErrorInvalidSigner, "unable to verify signer signature of EIP712 typed data") | ||
} |
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.
IMO, this is redundant. The effect is exactly the same with 236 - 250.
app/ante/eip712.go
Outdated
FeePayer: feePayer, | ||
} | ||
|
||
typedData, err := eip712.WrapTxToTypedData(ethermintCodec, extOpt.TypedDataChainID, msgs[0], txBytes, feeDelegation) |
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.
msgs[0]
here implied that we can only have 1 msg, but there is no associated check previously.
Maybe we should change
if len(msgs) == 0 {
return sdkerrors.Wrap(sdkerrors.ErrNoSignatures, "tx doesn't contain any msgs to verify signature")
}
to
if len(msgs) != 1
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.
Or is it possible to make the TypedData support multiple msgs in one tx?
Does the bfscli still work with the new signature mode? It might be necessary for internal quick test. We may need an e2e test to demonstrate how to sign with Metamask. Links might be useful:
|
app/ante/ante.go
Outdated
authante "github.com/cosmos/cosmos-sdk/x/auth/ante" | ||
) | ||
|
||
// NewAnteHandler returns an ante handler responsible for attempting to route a |
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.
A bit confuse about this PR. If we use Cosmos CLI to structure and send transactions, it will choose the signature method according to signMode. Just like the below code in cosmos-sdk.
func makeAuxSignerData(clientCtx client.Context, f Factory, msgs ...sdk.Msg) (tx.AuxSignerData, error) {
// ....
err = b.SetSignMode(f.SignMode())
if err != nil {
return tx.AuxSignerData{}, err
}
// ...
sig, _, err := clientCtx.Keyring.Sign(name, signBz)
if err != nil {
return tx.AuxSignerData{}, err
}
b.SetSignature(sig)
return b.GetAuxSignerData()
}
enum SignMode {
// ...
// Since: cosmos-sdk 0.45.2
SIGN_MODE_EIP_191 = 191;
}
Are we considering adding a SigMode to support EIP712? if not, we can't use the Cosmos CLI.
36dd7e7
to
915a145
Compare
Description
Add support for EIP712 and eth address standard
Rationale
In order that users could sign BFS transactions by a ethereum wallet like MetaMask directly, we adopt eth's secp256k1 and address rules as our keys algorithm. Besides, adding EIP712 support will make it more user friendly.
Example
N/A
Changes
Notable changes:
Address
implement,ETHAddress
, which is compatible of BSC addressReference
https://github.com/evmos/evmos/blob/main/app/ante/ante.go
https://github.com/evmos/evmos/blob/main/app/ante/handler_options.go
https://github.com/evmos/ethermint/blob/main/app/ante/eip712.go