refactor: make contract abstract over Borrow #2082

prestwich · 2023-01-27T15:41:45Z

Motivation

Contracts currently hold Arc<M> and have bound where M: Middleware . This requires managing arcs across a codebase, which imposes unpleasant requirements on binaries

Solution

Modify the contract object to be abstract over B: Borrow<M> instead of holding an Arc<M>. This allows contracts to be used directly with Cows, non-ARCed middleware, Boxes, other smart pointers, and anything else that uses the Borrow trait.
rename the modified object ContractInternal and create a type alias under the old name, for backwards compatibility

Example use case:

you can now use a contract directly with a Lazy<Provider<Http>> without having to have an arc in the lazy first

Inference Limitation

While FunctionCall and ContractInstance may be used with any type is Borrow<M>, there are sometimes multiple choices of borrowed type that satisfy the M: Middleware constraint.

because Middleware has an auto_impl(&, Box, Arc), references, boxes, and arcs have multiple Borrow options that satisfy the constraint.
E.g. Arc<M> has these impls:

impl Borrow<Arc<M>> for Arc<M>
impl Borrow<M> for Arc<M>
impl Borrow<&M> for Arc<M>

as a result, type inference fails when calling ContractInstance::new(address, arc_mware) users must instead specify ContractInstance::<_, ConcreteMiddlewareType>(address, arc_mware)

Removing the auto_impl will solve this. However, we don't know the extent of reliance on that auto_impl. As a result, using the backwards-compatibility aliases Contract and ContractCall is still recommended. the Followup work section describes a migration route to fix inference and deprecate the backwards-compatibility aliases

Followup work

Change the abigen derived Contract objects to use the generic ContractInternal
in 2.0.0, eliminate the type alias, and have the generic version use the Contract name

Lib-wide refactoring plan:

list further abstraction targets
- EthEvent
- ContractDeployer
- ContractFactory
- Middlewares (should the stack be MyMiddleware { inner: Borrow<M> } ?)
- ProviderOracle
- abigen! output code
refactor to use borrow
provide backwards compatible type aliases like type Contract = ContractInstance<Arc<M>, M>
remove auto_impl(&, Box, Arc) on Middleware
write migration guide
deprecate backwards-compatibility aliases
remove aliases

PR Checklist

Added Tests
Added Documentation
Updated the changelog
Breaking changes

ethers-contract/src/lib.rs

prestwich · 2023-01-27T15:45:16Z

ethers-contract/src/contract.rs

 }

-impl<M> std::ops::Deref for Contract<M> {
+pub type Contract<M> = ContractInternal<std::sync::Arc<M>, M>;


type aliases ensure backwards compatibility

mattsse

this makes it easier indeed,

especially for http provider, which actually could be made Clone

ethers-contract/src/contract.rs

prestwich · 2023-01-27T16:04:37Z

ethers-contract/src/contract.rs

            tx,
-            client: Arc::clone(&self.client), // cheap clone behind the Arc
+            client: self.client.clone(),


we could make ContractCallInternal borrow our B here, by introducing a lifetime to it

I am concerned about the practicality of the B: Clone bound, for borrow types without clone. are there commonly used borrow types that are NOT clone? 🤔

It can be the case when B: Borrow<M> is equal to M and M is not Clone, since Borrow has a blanket implemention for any T: ?Sized. Otherwise it would mostly be the standard smart pointers. So I'd say if all our Middleware implement Clone this is fine

unfortunately, not all of our middleware implement Clone, IIRC. e.g. I think SignerMiddleware<LedgerSigner, _> does not 🤔

Ah yeah right, I guess this will need to be specified in a doc somewhere that it is recommended wrapping M with a smart pointer or using references to avoid cloning data or for cases where that it is actually not possible

I'm going to make a couple toy examples for when B is M or &M. (&M is Clone however, so that should be fine)

I've also confirmed that NonceManagerMiddleware is not Clone

so the place where I user would run into this is:

instantiating a contract with an owned non-clone middleware

slight adjustment to screenshot. 3rd example is wrong, as M is &. corrected version here:

Is that a big problem? Can't we pass these Middlewares by reference, instead of owned? And otherwise, just Arc them?

Is that a big problem? Can't we pass these Middlewares by reference, instead of owned? And otherwise, just Arc them?

it's a not-immediately-obivous issue that users will run into and ask about. It compounds with the ambiguity caused by auto_imp(&, Box, Arc) to make this more-powerful API extremely difficult to use

if you call ContractInstance::new(address, &provider) you get a type ambiguity around M. is M Provider or &Provider. Both implement Middleware and both can be borrowed from &Provider. Same ambiguity for Arc

The follow up work (will copy this to original post) to get this usable by beginner rustaceans is something like:

1. list further abstraction targets 2. refactor to use borrow 3. provide backwards compatible type aliases like `type Contract = ContractInstance<Arc<M>, M>` 4. remove `auto_impl(&, Box, Arc)` on `Middleware` 5. write migration guide 6. deprecate backwards-compatibility aliases 7. remove aliases

ethers-contract/src/base.rs

prestwich · 2023-01-27T16:17:36Z

currently checking out the test breakage. looks like this will affect type inference

prestwich · 2023-01-27T16:24:40Z

error[E0283]: type annotations needed
   --> ethers-contract/tests/it/contract.rs:431:14
    |
431 |             .connect(client2.clone())
    |              ^^^^^^^ cannot infer type of the type parameter `N` declared on the associated function `connect`
    |
    = note: multiple `impl`s satisfying `Arc<ethers_providers::Provider<Http>>: Borrow<_>` found in the following crates: `alloc`, `core`:
            - impl<T> Borrow<T> for Arc<T>
              where T: ?Sized;
            - impl<T> Borrow<T> for T
              where T: ?Sized;
note: required by a bound in `ethers_contract::contract::ContractInternal::<B, M>::connect`
   --> /Users/james/devel/ethers-rs/ethers-contract/src/contract.rs:269:12
    |
269 |         C: Borrow<N>,
    |            ^^^^^^^^^ required by this bound in `ethers_contract::contract::ContractInternal::<B, M>::connect`
help: consider specifying the type arguments in the function call
    |
431 |             .connect::<C, N>(client2.clone())
    |                     ++++++++

the test failures are all driven by failed inference on connect. because connect is now generic over B, and B: Borrow<M> and M: Borrow<M>, both B and M are valid args for connect(). the compiler doesn't know whether it should be using the B or borrowing M from it and using M

this is only an issue when switching the middleware connected to a contract, and the solution is to write out connect::<Arc<_>, _>(). I think this is acceptable (although it is a breaking change)

prestwich · 2023-01-27T16:30:20Z

workaround:

changed params to connect to use an arc to leave existing code un-broken
added connect_with with the borrow api, which requires more type ascription

prestwich · 2023-01-27T17:02:14Z

anyone have a better name than ContractInternal ? 🤔

mattsse

I like this change,
using Borrow should allow us to create a Contract that uses a reference to a provider (&M) right? do we have an example for this.

so it's possible to

connect ws
create and consume Contract with &ws
return ownership of ws

so "Arc" should no longer be mandatory for every "Contract" interaction, right?

ethers-contract/src/call.rs

ethers-contract/src/contract.rs

mattsse · 2023-01-29T14:06:25Z

ethers-contract/src/contract.rs

+/// `Contract` is a [`ContractInternal`] object with an `Arc` middleware.
+/// This type alias exists to preserver backwards compatibility with
+/// non-abstract Contracts.
+///
+/// For full usage docs, see [`ContractInternal`]
+pub type Contract<M> = ContractInternal<std::sync::Arc<M>, M>;


should we perhaps mark the aliases as deprecated?

adding a deprecated tag results in close to 200 warnings in the ethers codebase. To avoid noise in the PR, I am adding in a commented-out deprecated attribute. I'll uncomment it and change all of our internal usage as a last step.

update:

see top comment for future plans wrt this. Aliases will not be deprecated in this PR. We will deprecate after we have a straightforward and beginner-friendly migration plan

ethers-contract/src/contract.rs

…ontractInstance

ethers-contract/src/contract.rs

gakonst · 2023-01-30T00:57:03Z

ethers-contract/src/contract.rs

            tx,
-            client: Arc::clone(&self.client), // cheap clone behind the Arc
+            client: self.client.clone(),


Is that a big problem? Can't we pass these Middlewares by reference, instead of owned? And otherwise, just Arc them?

prestwich · 2023-01-31T16:41:21Z

@gakonst @mattsse @DaniPopes

Please see updated notes at top of comment thread. I believe this is ready for final review and merge 🎉

gakonst

Supportive - pending @mattsse

ethers-contract/src/call.rs

Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>

mattsse

I think this is step in the right direction.

I guess there could be some quirks wrt Arc and its auto_impls but we can figure things out when we encounter them.

prestwich added the enhancement New feature or request label Jan 27, 2023

prestwich requested a review from gakonst January 27, 2023 15:41

prestwich self-assigned this Jan 27, 2023

prestwich commented Jan 27, 2023

View reviewed changes

ethers-contract/src/lib.rs Outdated Show resolved Hide resolved

prestwich commented Jan 27, 2023

View reviewed changes

mattsse reviewed Jan 27, 2023

View reviewed changes

prestwich commented Jan 27, 2023

View reviewed changes

ethers-contract/src/contract.rs Show resolved Hide resolved

prestwich commented Jan 27, 2023

View reviewed changes

ethers-contract/src/base.rs Outdated Show resolved Hide resolved

mattsse suggested changes Jan 29, 2023

View reviewed changes

prestwich added 9 commits January 29, 2023 13:51

refactor: contract abstract over Borrow

6843294

refactor: preserve old connect usage

43cff00

nit: remove commented out modules

3e242f8

chore: changelog

ac5d195

test: add compile check to tests

c174059

docs: add usage note to doc and make sure contractcall is visible

a9af764

fix: test compilation

33a3ea1

refactor: ContractCallInternal -> FunctionCall, ContractInternal -> C…

7c1d6f8

…ontractInstance

fix: Send IntoFuture

f4570ca

prestwich force-pushed the prestwich/borrow branch from cc9762e to f4570ca Compare January 29, 2023 18:55

gakonst reviewed Jan 30, 2023

View reviewed changes

prestwich added 2 commits January 31, 2023 11:29

nit: must-use on connect

cef8011

docs: remove deprecation warning in docstrings for type aliases

1525709

gakonst approved these changes Jan 31, 2023

View reviewed changes

ethers-contract/src/call.rs Outdated Show resolved Hide resolved

Update ethers-contract/src/call.rs

5ec4ccf

Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>

DaniPopes approved these changes Jan 31, 2023

View reviewed changes

This was referenced Feb 1, 2023

Refactor factories to use Borrow<M> #2103

Merged

Prestwich/event no lifetime #2105

Merged

mattsse approved these changes Feb 6, 2023

View reviewed changes

gakonst merged commit 0236de8 into master Feb 6, 2023

gakonst deleted the prestwich/borrow branch February 6, 2023 21:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: make contract abstract over Borrow #2082

refactor: make contract abstract over Borrow #2082

prestwich commented Jan 27, 2023 •

edited

Loading

prestwich Jan 27, 2023

mattsse left a comment

prestwich Jan 27, 2023

DaniPopes Jan 27, 2023

prestwich Jan 27, 2023

DaniPopes Jan 27, 2023

prestwich Jan 27, 2023

prestwich Jan 27, 2023

prestwich Jan 27, 2023

prestwich Jan 27, 2023

gakonst Jan 30, 2023

prestwich Jan 31, 2023

prestwich commented Jan 27, 2023

prestwich commented Jan 27, 2023

prestwich commented Jan 27, 2023

prestwich commented Jan 27, 2023

mattsse left a comment

mattsse Jan 29, 2023

prestwich Jan 29, 2023

prestwich Jan 31, 2023

gakonst Jan 30, 2023

prestwich commented Jan 31, 2023

gakonst left a comment

mattsse left a comment

refactor: make contract abstract over Borrow #2082

refactor: make contract abstract over Borrow #2082

Conversation

prestwich commented Jan 27, 2023 • edited Loading

Motivation

Solution

Inference Limitation

Followup work

PR Checklist

Choose a reason for hiding this comment

mattsse left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestwich commented Jan 27, 2023

prestwich commented Jan 27, 2023

prestwich commented Jan 27, 2023

prestwich commented Jan 27, 2023

mattsse left a 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

prestwich commented Jan 31, 2023

gakonst left a comment

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

prestwich commented Jan 27, 2023 •

edited

Loading