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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 10 commits into from
Sep 15, 2019
Merged

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

Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7cb9e68
Add `web3::contract::deploy::execute_no_unlock`
rockbmb Sep 8, 2019
c99e2bc
Refactor common parts of `execute` and `execute_no_unlock`
rockbmb Sep 9, 2019
6ac5d7d
Revert "Refactor common parts of `execute` and `execute_no_unlock`"
rockbmb Sep 11, 2019
b5b2f56
Generalize field of `PendingContract` to accept `Future`s (WIP)
rockbmb Sep 11, 2019
6f2f992
Extend `PendingContract` type to support `sign_and_execute`
rockbmb Sep 12, 2019
5c45bcd
Refactor
tomusdrw Sep 12, 2019
2204f5d
Apply cargo fmt
rockbmb Sep 12, 2019
0589a9b
Update `Future` implementation for `PendingContract`
rockbmb Sep 12, 2019
1df0720
Merge branch 'master' into execute-no-unlock
tomusdrw Sep 13, 2019
2cee697
Remove unnecessary type parameter from function
rockbmb Sep 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
57 changes: 51 additions & 6 deletions src/contract/deploy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,12 @@ impl<T: Transport> Builder<T> {
self
}

/// Execute deployment passing code and contructor parameters.
pub fn execute<P, V>(self, code: V, params: P, from: Address) -> Result<PendingContract<T>, ethabi::Error>
fn execute_core<P, V, F>(self, code: V, params: P, from: Address, f: F) -> Result<PendingContract<T>, ethabi::Error>
where
P: Tokenize,
V: AsRef<str>,
T: Transport,
F: Fn(TransactionRequest, T, core::time::Duration, usize) -> confirm::SendTransactionWithConfirmation<T>,
{
let options = self.options;
let eth = self.eth;
Expand Down Expand Up @@ -89,11 +90,11 @@ impl<T: Transport> Builder<T> {
condition: options.condition,
};

let waiting = confirm::send_transaction_with_confirmation(
eth.transport().clone(),
let waiting = f(
tx,
self.poll_interval,
self.confirmations,
eth.transport().clone(),
self.poll_interval.clone(),
self.confirmations.clone(),
);

Ok(PendingContract {
Expand All @@ -102,6 +103,50 @@ impl<T: Transport> Builder<T> {
waiting,
})
}

/// Execute deployment passing code and contructor parameters.
pub fn execute<P, V>(self, code: V, params: P, from: Address) -> Result<PendingContract<T>, ethabi::Error>
where
P: Tokenize,
V: AsRef<str>,
T: Transport,
{
let f = |tx, eth: T, poll_interval, confirmations| {
confirm::send_transaction_with_confirmation(eth, tx, poll_interval, confirmations)
};
self.execute_core(code, params, from, f)
}

/// Execute deployment passing code and contructor parameters.
///
/// Unlike the above `execute`, this method uses
/// `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>(
Copy link
Owner

Choose a reason for hiding this comment

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

Can we call it sign_and_execute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

self,
code: V,
params: P,
from: Address,
password: &str,
web3: crate::Web3<T>,
) -> Result<PendingContract<T>, ethabi::Error>
where
P: Tokenize,
V: AsRef<str>,
T: Transport,
{
let f = |tx, eth: T, poll_interval, confirmations| {
let raw_tx = web3
.personal()
.sign_transaction(tx, password)
.wait()
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@rockbmb rockbmb Sep 9, 2019 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 👍

Copy link
Contributor Author

@rockbmb rockbmb Sep 10, 2019 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

.expect("Transaction signing failed");
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right.


confirm::send_raw_transaction_with_confirmation(eth, raw_tx.raw, poll_interval, confirmations)
};
self.execute_core(code, params, from, f)
}
}

/// Contract being deployed.
Expand Down