Skip to content
This repository has been archived by the owner on Oct 19, 2024. It is now read-only.

Add ContractCall::call_owned(self) #1797

Closed
wants to merge 0 commits into from
Closed

Add ContractCall::call_owned(self) #1797

wants to merge 0 commits into from

Conversation

TmLev
Copy link
Contributor

@TmLev TmLev commented Oct 20, 2022

Motivation

I was trying to call multiple contracts concurrently by aggregating calls into a vec, so that I can join_all(calls).await them later.

Since ContractCall::call(&self) takes an immutable reference, compiler prevents future from being created because future references ContractCall, which may not live long enough.

I’ve tried Box-ing ContractCall, but it didn’t quite work.

Solution

Add another method that takes ownership: call_owned(self).
That completely solved my problem.

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog

@TmLev
Copy link
Contributor Author

TmLev commented Oct 20, 2022

Can update the changelog, if needed.

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

I also stumbled across this a couple of times.

this is useful if you need to move the future around.

An alternative could be to implement this if the type is Clone but I think contract.clone().call_owned` is better.

needs cargo + nightly fmt

@TmLev
Copy link
Contributor Author

TmLev commented Oct 21, 2022

Will do in a couple of minutes

@TmLev
Copy link
Contributor Author

TmLev commented Oct 21, 2022

Done

@TmLev
Copy link
Contributor Author

TmLev commented Oct 24, 2022

@gakonst maybe merge? :)

@prestwich
Copy link
Collaborator

can't you just wrap in an async move {} block that takes ownership?

@TmLev
Copy link
Contributor Author

TmLev commented Oct 25, 2022

Yeah, that should work. You think that call_owned(self) is redundant?

@prestwich
Copy link
Collaborator

tbh, I think that making call() consume self might be ideal?

@TmLev
Copy link
Contributor Author

TmLev commented Oct 25, 2022

Not necessarily?.. In case you want to estimate gas and do a transaction, after which transaction may fail and you want to retry?..

@prestwich
Copy link
Collaborator

well, the user intent for .call() is to read the chain. there's no usage I can think of in which the ContractCall is not dropped after the eth_call is executed. So fn call(self) is probably more in line with user expectation than fn call(&self)

@TmLev
Copy link
Contributor Author

TmLev commented Oct 25, 2022

In case it fails because of the provider and user wants to retry.

@prestwich
Copy link
Collaborator

ContractCall is Clone so that user could clone it before calling call() or send() 🤔

@prestwich
Copy link
Collaborator

I think much better dev ergonomics for joins is probably worth slightly worse dev ergo for retries? 🤔

@TmLev
Copy link
Contributor Author

TmLev commented Oct 26, 2022

Joins can be solved with async move { ... } blocks, as you said. .clone()-ing the call for retries is not ideal.

@prestwich
Copy link
Collaborator

slept on it. this would be a good place to implement IntoFuture, and keep call() and send() the same as they are now. my_call.into_future() is a lot more sensible to read than async move { my_call.call() }

@gakonst
Copy link
Owner

gakonst commented Oct 26, 2022

Same page as Prestwich, let's replace with IntoFuture.

@TmLev
Copy link
Contributor Author

TmLev commented Nov 2, 2022

ContractCall has three async methods:

  • estimate_gas
  • call
  • send

I'm not sure how IntoFuture can handle not just one, but all three of the use-cases.

Or are you suggesting to create new type (say, ContractCallFuture)? But what this type is supposed to do?

@gakonst
Copy link
Owner

gakonst commented Nov 3, 2022

I'd default to call() and ignore the other 2

@TmLev TmLev closed this Nov 3, 2022
@TmLev TmLev mentioned this pull request Nov 3, 2022
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants