search

immutability-io / vault-ethereum

A plugin that turns Vault into an Ethereum wallet.
243 stars 65 forks source link

adding optional "raw" modified to sign/verify APIs to allow signing of raw transaction hashes #3

Closed shayangz closed 6 years ago

shayangz commented 6 years ago

Description

This PR adds a new sign-raw API under accounts path to allow signing of raw transaction hashes.

This new API is modeled after the existing sign API, with the main difference being that input is a raw transaction already hashed instead of free-text message to be signed.

Motivation and Context

One of the benefits of using vault-ethereum plugin, is the ability to never need to expose secret information of accounts such as their private key in other parts of your infrastructure while still being able to perform all necessary actions against the accounts in question.

One such action is signing of variety of different transactions that one might want to execute against the network.

By adding this API, the caller can construct any raw transaction in their desired library/application and simply send the transaction binary hash to Vault for signature. Once she receives the signature back, it can be sent to desired Ethereum client node on the network, which will propagate the transaction out to other nodes. All of this can now happen without the private key needing to leave the secure walls of Vault.

How Has This Been Tested?

This is a brand new API and does not impact any existing code. I have tested creating raw transactions in web3j, signing using this API, and executing the signed transaction against a Ganache CLI v6.1.0 (ganache-core: 2.1.0) client successfully.

Types of changes

Checklist:

cypherhat commented 6 years ago

Thanks for contributing!

From what I can see, this is pretty much the same mechanism as the current sign with the omission of these lines:

current

msg := fmt.Sprintf("\x19Ethereum Signed Message:\n%d%s", len(input), input)
hash := crypto.Keccak256([]byte(msg))

and the addition of this:

this PR

input, err := hexutil.Decode(inputRaw)

As an alternative, how about we add a flag raw to the current sign API? This flag could default to false allowing the interface to stay unchanged.

What do you think?

shayangz commented 6 years ago

I agree that your suggested refactor would keep the code more concise, but I worry that it could lead to confusion. The semantics of the two APIs are very different. It also raises the question whether verify API should be modified as well or not.

I can make the change if you want, but I felt keeping them separate might have clarity benefits.

Let me know how you want to proceed.

cypherhat commented 6 years ago

I agree that the semantics are different but it seems more confusing to have 2 sign methods (to me.) If you are passionate they should be different, I will definitely listen - I am on the fence. (Also, glad you have tested with ganache - I had some issues in that regard.)

Well, I think there should be symmetric sign and verify commands in either regard. That was gonna be my other comment.

If you will attempt a symmetric version of verify (verify -raw ?) that would be cool.

shayangz commented 6 years ago

@cypherhat made the suggested changes. Let me know what you think.

cypherhat commented 6 years ago

Looks good. I'll cut a new release soon. Thank you.

DeRain commented 6 years ago

@cypherhat Hello there. Does it exist in the current master branch? I think signing transactions option was removed after big refactoring. Correct me if I'm wrong, please.

By the way - do you need this signTx method? :)

cypherhat commented 6 years ago

the new method is a raw signature - here is the API for sign:

DeRain commented 6 years ago

@cypherhat I've tried to get signed transaction via this method but failed. By the way - just added a more developer friendly method for signing transactions only - feel free to review https://github.com/immutability-io/vault-ethereum/pull/23 🤓