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

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

Merged
merged 16 commits into from Nov 19, 2019
Merged

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

merged 16 commits into from Nov 19, 2019

Conversation

nlordell
Copy link
Contributor

@nlordell nlordell commented Nov 6, 2019

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

@reuvenpo
Copy link
Contributor

reuvenpo commented Nov 11, 2019

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
Copy link
Owner

tomusdrw commented Nov 13, 2019

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

Copy link
Owner

@tomusdrw tomusdrw left a comment

Looks good! A bunch of code style grumbles though.

src/api/accounts.rs Show resolved Hide resolved
src/api/accounts.rs Show resolved Hide resolved
SignTransactionFuture::new(self, tx, key)
}

/// Hash a message. The data will be UTF-8 HEX decoded and enveloped as
Copy link
Owner

@tomusdrw tomusdrw Nov 17, 2019

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
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),
Copy link
Owner

@tomusdrw tomusdrw Nov 17, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@nlordell nlordell Nov 18, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Owner

@tomusdrw tomusdrw Nov 18, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@nlordell nlordell Nov 18, 2019

Choose a reason for hiding this comment

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

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.

#[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
Copy link
Owner

@tomusdrw tomusdrw Nov 17, 2019

Choose a reason for hiding this comment

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

❤️ Thank you for all the tests.

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

mod accounts;
Copy link
Owner

@tomusdrw tomusdrw Nov 17, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@nlordell nlordell Nov 18, 2019

Choose a reason for hiding this comment

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

Oops!

src/error.rs Show resolved Hide resolved
pub gas: Option<U256>,
/// Gas price (None for sensible default)
pub gas_price: Option<U256>,
/// Transfered value (None for no transfer)
Copy link
Owner

@tomusdrw tomusdrw Nov 17, 2019

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

@nlordell nlordell Nov 18, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Owner

@tomusdrw tomusdrw Nov 18, 2019

Choose a reason for hiding this comment

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

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

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

@tomusdrw tomusdrw Nov 17, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@nlordell nlordell Nov 18, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Owner

@tomusdrw tomusdrw Nov 18, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@nlordell nlordell Nov 18, 2019

Choose a reason for hiding this comment

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

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

}

/// 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>
Copy link
Owner

@tomusdrw tomusdrw Nov 17, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@nlordell nlordell Nov 18, 2019

Choose a reason for hiding this comment

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

Oops! That was a bit of an oversight.

Copy link
Contributor Author

@nlordell nlordell Nov 18, 2019

Choose a reason for hiding this comment

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

@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?

Copy link
Owner

@tomusdrw tomusdrw Nov 18, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@nlordell nlordell Nov 18, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@nlordell nlordell Nov 18, 2019

Choose a reason for hiding this comment

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

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
Copy link
Owner

tomusdrw commented Nov 17, 2019

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
Copy link
Contributor

reuvenpo commented Nov 18, 2019

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
Copy link
Contributor Author

nlordell commented Nov 18, 2019

@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 😄

@nlordell nlordell requested a review from tomusdrw Nov 18, 2019
Copy link
Owner

@tomusdrw tomusdrw left a comment

Looks good!

/// 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
Copy link
Owner

@tomusdrw tomusdrw Nov 19, 2019

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
#[derive(Clone, Debug, PartialEq)]
pub enum RecoveryMessage {
/// Message string
String(String),
Copy link
Owner

@tomusdrw tomusdrw Nov 19, 2019

Choose a reason for hiding this comment

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

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

Copy link
Owner

@tomusdrw tomusdrw Nov 19, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@reuvenpo reuvenpo Nov 19, 2019

Choose a reason for hiding this comment

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

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

Copy link
Owner

@tomusdrw tomusdrw Nov 19, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@nlordell nlordell Nov 21, 2019

Choose a reason for hiding this comment

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

Looks like @reuvenpo beat me to it!

@tomusdrw tomusdrw merged commit 5e8e0a4 into tomusdrw:master Nov 19, 2019
1 check passed
@reuvenpo reuvenpo mentioned this pull request Nov 20, 2019
tomusdrw added a commit that referenced this issue Jan 27, 2020
tomusdrw added a commit that referenced this issue Jan 28, 2020
tomusdrw pushed a commit that referenced this issue Feb 11, 2020
* 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
Copy link

faea726 commented Apr 29, 2022

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.

image

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

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

Successfully merging this pull request may close these issues.

4 participants