Add transaction signing with in `accounts` sub-namespace. #279

nlordell · 2019-11-06T15:04:41Z

Also adds a few related methods like hash_message, sign and recover. This closes #116.

reuvenpo · 2019-11-11T10:52:29Z

Hey there @nlordell, I couldn't find a way to contact you so I'm writing this here.

My team is trying to achieve this exact functionality this week so that we can use our own private key instead of relying on unlocked accounts on ethereum nodes.
We have a very ugly wrapper code which uses ethereum-tx-sign plus parts of this crate (web3) that we're working on. I figured we couldn't be the first team to need this functionality, so i decided to check this repo's pull request list, and was really happy to see this work is already being done 😄

We want to ask when can we expect this to be merged into master and published in a new release?

We also have a question that may be better asked of @tomusdrw: what is the best way, given a contract's abi, to sign calls to the contract's methods? ideally, we would like to have a signed_call_with_confirmations method similar to web3::contract::Contract::call_with_confirmations which would use the functionality introduced in this PR.

tomusdrw · 2019-11-13T13:49:38Z

Hi guys! I didn't have time to look closely at the PR but will definitely do that in the upcoming 36h.

tomusdrw

Looks good! A bunch of code style grumbles though.

src/api/accounts.rs

tomusdrw · 2019-11-17T22:47:03Z

src/api/accounts.rs

+        SignTransactionFuture::new(self, tx, key)
+    }
+
+    /// Hash a message. The data will be UTF-8 HEX decoded and enveloped as


Can you change the doc comments to follow a pattern:

/// <description summary> /// /// <more elaborate documentation>

So here it would be:

/// Hash a message according to EIP-191. /// /// The data will be UTF-8 HEX encoded and enveloped as follows...

src/api/accounts.rs

tomusdrw · 2019-11-17T22:50:08Z

src/api/accounts.rs

+            to: self.tx.to.unwrap_or_default(),
+            value: self.tx.value.unwrap_or_default(),
+            gas_price,
+            gas: self.tx.gas.unwrap_or(TRANSACTION_BASE_GAS_FEE),


21k will really never be enough to cover the transaction. I think we should rather default to something higher like 100k.

I chose 21K as this is the base transaction amount and exactly what it uses to send eth from one address to another (with no contracts involved). I think if we want to be extra cleaver, we could estimate the gas price when it is not provided.

I'd leave the estimation to the user - we can suggest them to run estimateGas before making a call. BTW is it easy to go from the payload required for estimateGas and the payload that goes to this function? We should consider adding a From implementations to make such workflow easier.

Indeed 21k is ok for simple transfers, but if only you'll try to send some data, we will need more gas. I'm still in favour of increasing the "default sensible" gas here - the user will get the rest refunded anyway and I think it can prevent a class of simple errors. Alternatively we can make gas a required field and force the user to always set it.

FWIW, web3js requires gas parameter for web3.eth.accounts.signTransaction. I like having a default gas as I find it makes things more ergonomic (so we can implement Default for TransactionParameters). I think I'll hike it up to 100k then.

I'll add the Into and From impls between TransactionParameters and CallRequest which is used for estimate gas.

tomusdrw · 2019-11-17T23:08:29Z

src/api/accounts.rs

+    #[test]
+    fn raw_tx_params_sign() {
+        // retrieved test vector from:
+        // https://web3js.readthedocs.io/en/v1.2.2/web3-eth-accounts.html#eth-accounts-signtransaction


❤️ Thank you for all the tests.

tomusdrw · 2019-11-17T23:08:57Z

src/api/mod.rs

@@ -1,5 +1,6 @@
 //! `Web3` implementation

+mod accounts;


Please re-export accounts::Accounts otherwise the docs are not visible.

src/error.rs

tomusdrw · 2019-11-17T23:11:05Z

src/types/signed.rs

+    pub gas: Option<U256>,
+    /// Gas price (None for sensible default)
+    pub gas_price: Option<U256>,
+    /// Transfered value (None for no transfer)


Why None and not H256::zero() or Default::default()? I think it makes it really confusing if there is any difference between None and Some(0) (afaik there isn't).

For gas price specifically there is a difference. Some(0) means that you want to pay 0 for gas (transaction will never be mined though). None on the other hand will tell the signing function to get the current recommended gas price and use that.

Yes, I only meant value and data field. The rest is fine.

tomusdrw · 2019-11-17T23:11:20Z

src/types/signed.rs

+    pub gas_price: Option<U256>,
+    /// Transfered value (None for no transfer)
+    pub value: Option<U256>,
+    /// Data (None for empty data)


Same here. I think it would be best to drop the Option.

Sounds good. We can drop Option for to, value, and data. I had used Option originally to signal that it providing this transaction parameter was optional and when it was not provided some default would be used. For the aforementioned fields, the default also happens to be Default::default() so we can get rid of Option for them.

I'd keep it for to as well. There is a difference between contract creation and sending to 0x0 address.

Ah I see it in ethereum_transaction, it gets encoded differently if it's there or if its missing.

tomusdrw · 2019-11-17T23:14:13Z

src/api/accounts.rs

+    }
+
+    /// Recovers the Ethereum address which was used to sign the given data.
+    pub fn recover<S, Sig>(&self, message: S, signature: Sig) -> Result<Address, Error>


Might be useful to have another recover for raw hashes? Maybe make the message an enum and allow passing etiher Hash or AsRef<str>?

Oops! That was a bit of an oversight.

@tomusdrw Actually, thinking about it now it would be nice to be able to do something like:

let signed_data = accounts.sign("hello world", &key); let address = accounts.recover(signed_data)?; assert_eq!(address, key.public().address()); let signed_tx = accounts.sign(tx, &key).await?; let address = accounts.recover(signed_tx)?; assert_eq!(address, key.public().address());

What do you think?

Yes, but how do you recover something you didn't sign yourself? Also since we don't have specialisation either the method names need to be different or you need to accept impl Into<SomeEnum> as a parameter.

I went ahead and implemented this. If you don't like it, let me know and I can change it.

Sorry, for some reason I didn't see you message when I commented. Yes you can recover things that you did not sign yourself. A Recovery struct with the recovery data can be constructed fairly easily, we do this in one of the unit tests.

Let me know if you like how I implemented it, if not I can change it to something else.

tomusdrw · 2019-11-17T23:18:13Z

ideally, we would like to have a signed_call_with_confirmations method similar to web3::contract::Contract::call_with_confirmations which would use the functionality introduced in this PR.

Happy to accept a PR introducing this feature @reuvenpo.

reuvenpo · 2019-11-18T09:07:35Z

ideally, we would like to have a signed_call_with_confirmations method similar to web3::contract::Contract::call_with_confirmations which would use the functionality introduced in this PR.

Happy to accept a PR introducing this feature @reuvenpo.

I'll build it on this PR, so i'll wait until this one has finished cooking :)

nlordell · 2019-11-18T15:15:17Z

@tomusdrw I think I addressed all of your comments. Note that I changed how recovery works, let me know what you think of the new implementation. Take a look at the accounts_recovery* tests to see how it works 😄

tomusdrw

Looks good!

tomusdrw · 2019-11-19T11:19:21Z

src/api/accounts.rs

+    /// The data is a UTF-8 encoded string and will enveloped as follows:
+    /// `"\x19Ethereum Signed Message:\n" + message.length + message` and hashed
+    /// using keccak256.
+    pub fn hash_message<S>(&self, message: S) -> H256


We can now use impl Trait for this:

Suggested change

pub fn hash_message<S>(&self, message: S) -> H256

pub fn hash_message(&self, message: impl AsRef<str>) -> H256 {

Most likely it applies to every generic method introduced in this PR. But it's not critical, just a hint :)

src/error.rs

tomusdrw · 2019-11-19T11:23:28Z

src/types/recovery.rs

+#[derive(Clone, Debug, PartialEq)]
+pub enum RecoveryMessage {
+    /// Message string
+    String(String),


Shouldn't that be Data(Bytes)? I think you can sign arbitraty (non-utf8) bytes.

I see that we have restricted hashing to AsRef<str> anyway, so it's good as is.

Wait a second, that's a good point - shouldn't this crate allow hashing arbitrary data?

Yeah, feel free to do a follow up PR on this, or maybe @nlordell is interested :)

Looks like @reuvenpo beat me to it!

…)" This reverts commit 5e8e0a4.

…)" (#315) This reverts commit 5e8e0a4.

* Revert "Revert "Add transaction signing with in `accounts` sub-namespace. (#279)" (#315)" This reverts commit 9928849. * implemented signing and recovery with secp256k1 crate * attempt to protect key from being leaked

faea726 · 2022-04-29T15:35:25Z

Here is my code:

use secp256k1::SecretKey;
use std::str::FromStr;

use web3::{
    contract::{Contract, Options},
    signing::SecretKeyRef,
    types::{Address, U256},
};

#[tokio::main]
async fn main() -> web3::Result<()> {
    let http_provider = web3::transports::Http::new("https://bscrpc.com")?;
    let w3 = web3::Web3::new(http_provider);

    // Insert the 32-byte private key in hex format (do NOT prefix with 0x)
    let prvk =
        SecretKey::from_str("XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX")
            .unwrap();

    let keyRef = SecretKeyRef::new(&prvk);

    Ok(())
}

And I got an error at the last line as attached image.

I tried to switch secp256k1 to version 0.21 as cargo.toml. But it cannot switch to that old version.

nlordell mentioned this pull request Nov 6, 2019

Move signing functionality from ethcontract upstream. cowprotocol/ethcontract-rs#14

Closed

reuvenpo mentioned this pull request Nov 14, 2019

Change chain_id from u8 to u32 synlestidae/ethereum-tx-sign#7

Closed

tomusdrw reviewed Nov 17, 2019

View reviewed changes

nlordell added 6 commits November 18, 2019 09:38

implemented accounts.sign and accounts.hashMessage

bbcda00

Started adding transaction signing

298ab2c

rustfmt

4733a34

added unit test for IntoAccounts trait

67d20b9

added raw transaction parameter signing

e1b52bb

added transaction signing

9ff14c1

nlordell added 8 commits November 18, 2019 11:50

used ethereum-transaction instead of custom types

8ed6a60

cargo fmt

746d107

remove dead code

8eae8fc

add TODO remove the mem::transmute

15e6f7a

into signature changes

b0af0b0

added new recovery data type

85c1cb9

cargo fmt and clippy

dc3ac59

update to latest ethereum-transaction

9b98bc0

nlordell requested a review from tomusdrw November 18, 2019 15:15

nlordell added 2 commits November 18, 2019 16:52

rename types, more clippy

d0bdcd1

Added ability to get recovery data from raw signature

eca84ae

tomusdrw approved these changes Nov 19, 2019

View reviewed changes

tomusdrw merged commit 5e8e0a4 into tomusdrw:master Nov 19, 2019

tomusdrw mentioned this pull request Nov 20, 2019

Create account from private key #286

Closed

reuvenpo mentioned this pull request Nov 20, 2019

Custom signing methods #287

Merged

tomusdrw added a commit that referenced this pull request Jan 27, 2020

Revert "Add transaction signing with in accounts sub-namespace. (#279…

5b70049

…)" This reverts commit 5e8e0a4.

tomusdrw mentioned this pull request Jan 27, 2020

Revert "Add transaction signing with in accounts sub-namespace. #315

Merged

tomusdrw added a commit that referenced this pull request Jan 28, 2020

Revert "Add transaction signing with in accounts sub-namespace. (#279…

9928849

…)" (#315) This reverts commit 5e8e0a4.

tomusdrw mentioned this pull request Feb 1, 2020

Sign data with private key #320

Closed

nlordell mentioned this pull request Feb 5, 2020

Reimplement transaction signing with license compatible crate #325

Merged

	pub fn hash_message<S>(&self, message: S) -> H256
	pub fn hash_message(&self, message: impl AsRef<str>) -> H256 {

Add transaction signing with in accounts sub-namespace. #279

Add transaction signing with in accounts sub-namespace. #279

Conversation

nlordell commented Nov 6, 2019

reuvenpo commented Nov 11, 2019 • edited

tomusdrw commented Nov 13, 2019

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlordell Nov 18, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw commented Nov 17, 2019

reuvenpo commented Nov 18, 2019

nlordell commented Nov 18, 2019

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faea726 commented Apr 29, 2022 • edited

Add transaction signing with in `accounts` sub-namespace. #279

Add transaction signing with in `accounts` sub-namespace. #279

reuvenpo commented Nov 11, 2019 •

edited

nlordell Nov 18, 2019 •

edited

faea726 commented Apr 29, 2022 •

edited