Add `web3::contract::deploy::execute_no_unlock` #252

rockbmb · 2019-09-08T14:31:46Z

This PR introduces a variant of web3::contract::deploy::execute that does not require the account for which it is used to be unlocked.

This function is a variant of `web3::contract::deploy::execute` that does not require the account for which it is used to be unlocked.

Introduced in tomusdrw/rust-web3#252.

tomusdrw

The overall idea is ok, but there are few improvements to be made.

tomusdrw · 2019-09-09T07:28:32Z

src/contract/deploy.rs

+            let raw_tx = web3
+                .personal()
+                .sign_transaction(tx, password)
+                .wait()


That is making a synchronous call, it's not acceptable in async context.

Then how should I fetch the value that web3.personal().sign_transaction() returns?

I can chain this computation with and_then(move |signed_tx| {send_raw_transaction_with_confirmation ... }, but that's only moving the Future down the line, and the closure can't return one.

Almost forgot: the impl<T: Transport> Future for SendTransactionWithConfirmation<T> has type Item = TransactionReceipt, meaning even if the above is done, we will still be getting a TransactionReceipt where we should be getting a SendTransactionWithConfirmation<T> to be used to create a PendingContract.

Most likely we need to change PendingContract to be able to work with aribtrary Future that resolves to TransactionReceipt:

PendingContract< T: Transport, F: Future<Item=TransactionReceipt, Error=...> = SendTransactionWithConfirmation<T>, > { }

and then use the and_thencombinator as you've mentioned.

I was reluctant to change PendingContract without the go-ahead of someone more knowledgeable about the code base than me. If it sounds reasonable, then I will do so, and re-request a review.

Thank you for your attention 👍

I understand that the type signatures you wrote here are merely suggestive, but what does F: Future<Item=TransactionReceipt, Error=...> = SendTransactionWithConfirmation<T> mean?

I have not come across this syntax before.

It's a generic parameter, but with a default value. So that PendingContract<T> would still be backward compatible, as the F type would default to SendTransactionWithConfirmation if omitted.

I split the two functions back again since the previous refactoring would not work with this change to PendingContract.

However, now I'm getting an error on this line I am not sure I understand:

F mismatched types expected type parameter, found struct `futures::AndThen` note: expected type `Ft` found type `futures::AndThen<helpers::CallFuture<types::transaction::RawTransaction, <T as Transport>::Out>, confirm::SendTransactionWithConfirmation<T>, [closure@src\contract\deploy.rs:165:79: 172:10 eth:_, self:_]>`rustc(E0308) deploy.rs(177, 13): expected type parameter, found struct `futures::AndThen`

I was under the impression and_then returned a struct, AndThen, that implemented Future so I am not sure why PendingContract would have issues with it.

Do you mind lending me a hand with this? I apologise for my unreasonable request.

The thing is that you let the caller specify Ft, since it's a generic parameter, but you try to return AndThen later on.

So the error is just saying that the caller expected a custom Ft type, but you tried to force AndThen on them. Try using impl Future in the returned type of PendingContract. If not we can just put Box there.

tomusdrw · 2019-09-09T07:29:12Z

src/contract/deploy.rs

+    /// `sign_raw_transaction_with_confirmation` instead of
+    /// `sign_transaction_with_confirmation`, which requires the account from
+    /// which the transaction is sent to be unlocked.
+    pub fn execute_no_unlock<P, V>(


Can we call it sign_and_execute?

tomusdrw · 2019-09-09T07:29:35Z

src/contract/deploy.rs

+                .personal()
+                .sign_transaction(tx, password)
+                .wait()
+                .expect("Transaction signing failed");


We definitely should not panic if we can't sign the transaction - it should just be a regular Err that can be later handled by the caller.

You are right.

This reverts commit c99e2bc.

tomusdrw · 2019-09-12T10:16:10Z

src/contract/deploy.rs

+        from: Address,
+        password: &str,
+        web3: crate::Web3<T>,
+    ) -> Result<PendingContract<T, Ft>, ethabi::Error>


Suggested change

) -> Result<PendingContract<T, Ft>, ethabi::Error>

) -> Result<PendingContract<T, impl Future<...>>, ethabi::Error>

Drop Ft and try this.

mistake

rockbmb · 2019-09-12T12:21:25Z

@tomusdrw thank you for all the help.

I don't quite like the way sign_and_execute duplicates most of its code, but this can be merged if you approve of it. I can return to it some other time to refactor things.

tomusdrw · 2019-09-12T13:10:52Z

Have a look at: rockbmb#1

rockbmb · 2019-09-12T23:54:24Z

I only now realized I still need to update the Future instance for PendingContract, please do not merge yet.

tomusdrw · 2019-09-13T07:40:34Z

@rockbmb good to go now?

rockbmb · 2019-09-13T08:08:56Z

@tomusdrw not quite. I'm getting an odd compiler error at sign_and_execute's callsite, and I can't quite figure it out.

I would like to be sure it is not because of anything I did here.

tomusdrw · 2019-09-13T08:32:26Z

Maybe add an example/test to this PR so that we can debug it together?

rockbmb · 2019-09-13T08:48:03Z

@tomusdrw I'll add one right next to the test for execute, but sadly this will have to wait - I'll be afk for a few hours.

geigerzaehler · 2019-09-13T11:09:42Z

src/contract/deploy.rs

+    /// `sign_raw_transaction_with_confirmation` instead of
+    /// `sign_transaction_with_confirmation`, which requires the account from
+    /// which the transaction is sent to be unlocked.
+    pub fn sign_and_execute<P, V, Ft>(


The Ft parameter is unused. This is problematic for calling code because the compiler cannot infer the instantiation of Ft. This leads to the calling code always requiring an explicit type annotation.

Let's just remove it then. I guess I left it by mistake.

I'll try this out.

Does it solve the issues you were having?

It did. I get other errors now, but as far as this PR goes, it has what we need.
Do you still want me to add a test before we merge?

tomusdrw · 2019-09-15T08:28:30Z

@rockbmb Feel free to add a test in a follow-up PR.

Add web3::contract::deploy::execute_no_unlock

7cb9e68

This function is a variant of `web3::contract::deploy::execute` that does not require the account for which it is used to be unlocked.

rockbmb force-pushed the execute-no-unlock branch from 179a9f0 to 7cb9e68 Compare September 8, 2019 15:13

rockbmb marked this pull request as ready for review September 8, 2019 15:58

rockbmb added a commit to oscoin/oscoin-parity-wasm-prototype that referenced this pull request Sep 8, 2019

Refactor code to use execute_no_unlock

da0a419

Introduced in tomusdrw/rust-web3#252.

rockbmb added a commit to oscoin/oscoin-parity-wasm-prototype that referenced this pull request Sep 8, 2019

Refactor code to use execute_no_unlock

b3b31a0

Introduced in tomusdrw/rust-web3#252.

rockbmb added a commit to oscoin/oscoin-parity-wasm-prototype that referenced this pull request Sep 8, 2019

Refactor code to use execute_no_unlock

70beab9

Introduced in tomusdrw/rust-web3#252.

rockbmb added a commit to oscoin/oscoin-parity-wasm-prototype that referenced this pull request Sep 8, 2019

Refactor code to use execute_no_unlock

d9112a6

Introduced in tomusdrw/rust-web3#252.

rockbmb added a commit to oscoin/oscoin-parity-wasm-prototype that referenced this pull request Sep 8, 2019

Refactor code to use execute_no_unlock

3a4cf5a

Introduced in tomusdrw/rust-web3#252.

rockbmb added a commit to oscoin/oscoin-parity-wasm-prototype that referenced this pull request Sep 8, 2019

Refactor code to use execute_no_unlock

2887dc7

Introduced in tomusdrw/rust-web3#252.

rockbmb added a commit to oscoin/oscoin-parity-wasm-prototype that referenced this pull request Sep 8, 2019

Refactor code to use execute_no_unlock

376214f

Introduced in tomusdrw/rust-web3#252.

rockbmb added a commit to oscoin/oscoin-parity-wasm-prototype that referenced this pull request Sep 8, 2019

Refactor code to use execute_no_unlock

efe47b7

Introduced in tomusdrw/rust-web3#252.

Refactor common parts of execute and execute_no_unlock

c99e2bc

tomusdrw requested changes Sep 9, 2019

View reviewed changes

rockbmb added 2 commits September 11, 2019 20:07

Revert "Refactor common parts of execute and execute_no_unlock"

6ac5d7d

This reverts commit c99e2bc.

Generalize field of PendingContract to accept Futures (WIP)

b5b2f56

tomusdrw previously approved these changes Sep 12, 2019

View reviewed changes

tomusdrw self-requested a review September 12, 2019 10:16

Extend PendingContract type to support sign_and_execute

6f2f992

rockbmb force-pushed the execute-no-unlock branch from 4de83e3 to 6f2f992 Compare September 12, 2019 11:47

Refactor

5c45bcd

Apply cargo fmt

2204f5d

rockbmb force-pushed the execute-no-unlock branch from 9184442 to 2204f5d Compare September 12, 2019 23:27

rockbmb and others added 2 commits September 13, 2019 00:59

Update Future implementation for PendingContract

0589a9b

Merge branch 'master' into execute-no-unlock

1df0720

geigerzaehler reviewed Sep 13, 2019

View reviewed changes

Remove unnecessary type parameter from function

2cee697

tomusdrw approved these changes Sep 15, 2019

View reviewed changes

tomusdrw merged commit 48267cb into tomusdrw:master Sep 15, 2019

rockbmb deleted the execute-no-unlock branch September 15, 2019 12:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `web3::contract::deploy::execute_no_unlock` #252

Add `web3::contract::deploy::execute_no_unlock` #252

rockbmb commented Sep 8, 2019

tomusdrw left a comment

tomusdrw Sep 9, 2019

rockbmb Sep 9, 2019

rockbmb Sep 9, 2019 •

edited

tomusdrw Sep 9, 2019

rockbmb Sep 9, 2019

rockbmb Sep 10, 2019 •

edited

tomusdrw Sep 10, 2019

rockbmb Sep 11, 2019

tomusdrw Sep 12, 2019

tomusdrw Sep 9, 2019

rockbmb Sep 9, 2019

tomusdrw Sep 9, 2019

rockbmb Sep 9, 2019

tomusdrw Sep 12, 2019

rockbmb commented Sep 12, 2019

tomusdrw commented Sep 12, 2019

rockbmb commented Sep 12, 2019

tomusdrw commented Sep 13, 2019

rockbmb commented Sep 13, 2019 •

edited

tomusdrw commented Sep 13, 2019

rockbmb commented Sep 13, 2019

geigerzaehler Sep 13, 2019

tomusdrw Sep 13, 2019

rockbmb Sep 13, 2019

tomusdrw Sep 13, 2019

rockbmb Sep 13, 2019

tomusdrw commented Sep 15, 2019

	) -> Result<PendingContract<T, Ft>, ethabi::Error>
	) -> Result<PendingContract<T, impl Future<...>>, ethabi::Error>

Add web3::contract::deploy::execute_no_unlock #252

Add web3::contract::deploy::execute_no_unlock #252

Conversation

rockbmb commented Sep 8, 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

rockbmb Sep 9, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rockbmb Sep 10, 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

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

rockbmb commented Sep 12, 2019

tomusdrw commented Sep 12, 2019

rockbmb commented Sep 12, 2019

tomusdrw commented Sep 13, 2019

rockbmb commented Sep 13, 2019 • edited

tomusdrw commented Sep 13, 2019

rockbmb commented Sep 13, 2019

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

tomusdrw commented Sep 15, 2019

Add `web3::contract::deploy::execute_no_unlock` #252

Add `web3::contract::deploy::execute_no_unlock` #252

rockbmb Sep 9, 2019 •

edited

rockbmb Sep 10, 2019 •

edited

rockbmb commented Sep 13, 2019 •

edited