SignerMiddleware::send_transaction() drops the from field

[wolflo]

Version
Latest master branch (985509a).

Description

Current Behavior

SignerMiddleware::send_transaction() sends a transaction from it's signer regardless of the from field in the TransactionRequest.

The from address gets replaced with the signer address in sign_transaction(). I think that makes sense in the context of sign_transaction(), but it would be more intuitive if send_transaction() made an effort to preserve the TransactionRequest.

ethers-rs/ethers-middleware/src/signer.rs

Lines 147 to 150 in 985509a

	Ok(Transaction {
	hash: hash.into(),
	nonce,
	from: self.address(),

Expected Behavior

I would expect something like the following:

send_transaction honors the from field. If it receives a transaction with a from field set to an address other than the signer, it delegates to self.inner.send_transaction(). sign_transaction() in this case would either be unchanged or would return an error if asked to sign a transaction with from != self.address.

Test cases

The first shows the behavior in send_transaction() and the second shows sign_transaction(). If you put these into the tests mod in signer.rs they should be runnable with cargo test signer.

    #[tokio::test]
    async fn send_transaction_drops_from_field() {
        use ethers_core::utils::Ganache;

        // launch ganache
        let ganache = Ganache::new().spawn();

        // grab 2 wallets
        let signer: LocalWallet = ganache.keys()[0].clone().into();
        let from: LocalWallet = ganache.keys()[1].clone().into();

        // connect to the network
        let provider = Provider::try_from(ganache.endpoint())
            .unwrap();
        let provider = SignerMiddleware::new(provider, signer.clone());

        // craft the transaction
        let request = TransactionRequest::new();
        let request = request.from(from.address());

        // send it!
        let receipt = provider
            .send_transaction(request.clone(), None)
            .await.unwrap()
            .await.unwrap().unwrap();

        // retrieve the transaction that was sent
        let sent_tx = provider
            .get_transaction(receipt.transaction_hash)
            .await.unwrap().unwrap();

        // panics
        assert_eq!(sent_tx.from, request.from.unwrap());
    }

    #[tokio::test]
    async fn sign_transaction_accepts_from_not_signer() {
        use ethers_core::types::Address;

        // build a transaction with a from field
        let from = "0x863DF6BFa4469f3ead0bE8f9F2AAE51c91A907b4"
            .parse::<Address>()
            .unwrap();
        let tx = TransactionRequest::new().nonce(0).gas_price(0).gas(0);
        let tx = tx.from(from);

        // new SignerMiddleware with signer != from field of the transaction
        let provider = Provider::try_from("http://localhost:8545").unwrap();
        let key = LocalWallet::new(&mut rand::thread_rng());
        let client = SignerMiddleware::new(provider, key);

        // sign the transaction
        let signed_tx = client.sign_transaction(tx.clone()).await.unwrap();

        // panics
        assert_eq!(signed_tx.from, from);
    }

[gakonst]

Good catch - agree with the suggested fix of skipping the signing step if from is already set.