Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

rpc, evm: secure tx signing #20

Merged
merged 4 commits into from
May 12, 2021
Merged

rpc, evm: secure tx signing #20

merged 4 commits into from
May 12, 2021

Conversation

fedekunze
Copy link
Contributor

@fedekunze fedekunze commented May 12, 2021

Closes: #XXX

Description

Enable eth_sendTransaction and eth_sign rpc endpoints without passing the private key to the JSON-RPC service and relying solely on the keyring.


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@fedekunze fedekunze changed the title Fedekunze/signing rpc, evm: secure tx signing May 12, 2021
@fedekunze fedekunze marked this pull request as ready for review May 12, 2021 13:07
@fedekunze fedekunze merged commit 65453e4 into main May 12, 2021
@fedekunze fedekunze deleted the fedekunze/signing branch May 12, 2021 13:08
func (privKey PrivKey) Sign(msg []byte) ([]byte, error) {
return ethcrypto.Sign(ethcrypto.Keccak256Hash(msg).Bytes(), privKey.ToECDSA())
func (privKey PrivKey) Sign(digestBz []byte) ([]byte, error) {
if len(digestBz) != ethcrypto.DigestLength {
Copy link

@xlab xlab Aug 20, 2021

Choose a reason for hiding this comment

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

Hello. This conditional signing makes it different from the previous codebase we had. And introduces discrepancy with many ethermint-based codebases e.g. informalsystems/hermes#1295 (review) where we made our Inj chain compatible.

And actually in my opinion this can lead to very shady bugs when one expects always a keccak, but if input matches 32 bytes (and IS NOT keccak), it will implicitly be omitted. I never saw that in other projects (cosmos / ethereum related).

Choose a reason for hiding this comment

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

Ack

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants