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

refactor: make contract abstract over Borrow #2082

Merged
merged 12 commits into from
Feb 6, 2023
Merged

Conversation

prestwich
Copy link
Collaborator

@prestwich prestwich commented Jan 27, 2023

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:

  1. list further abstraction targets
    • EthEvent
    • ContractDeployer
    • ContractFactory
    • Middlewares (should the stack be MyMiddleware { inner: Borrow<M> } ?)
    • ProviderOracle
    • abigen! output code
  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

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog
  • Breaking changes

@prestwich prestwich added the enhancement New feature or request label Jan 27, 2023
@prestwich prestwich self-assigned this Jan 27, 2023
ethers-contract/src/lib.rs Outdated Show resolved Hide resolved
}

impl<M> std::ops::Deref for Contract<M> {
pub type Contract<M> = ContractInternal<std::sync::Arc<M>, M>;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

type aliases ensure backwards compatibility

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.

this makes it easier indeed,

especially for http provider, which actually could be made Clone

tx,
client: Arc::clone(&self.client), // cheap clone behind the Arc
client: self.client.clone(),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've also confirmed that NonceManagerMiddleware is not Clone

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

  • instantiating a contract with an owned non-clone middleware

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Screenshot 2023-01-27 at 12 27 58 PM

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

@prestwich
Copy link
Collaborator Author

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

@prestwich
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

anyone have a better name than ContractInternal ? 🤔

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 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

  1. connect ws
  2. create and consume Contract with &ws
  3. return ownership of ws

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

ethers-contract/src/call.rs Outdated Show resolved Hide resolved
ethers-contract/src/contract.rs Outdated Show resolved Hide resolved
Comment on lines 178 to 183
/// `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>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we perhaps mark the aliases as deprecated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
ethers-contract/src/contract.rs Show resolved Hide resolved
tx,
client: Arc::clone(&self.client), // cheap clone behind the Arc
client: self.client.clone(),
Copy link
Owner

Choose a reason for hiding this comment

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

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

@prestwich
Copy link
Collaborator Author

@gakonst @mattsse @DaniPopes

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

Copy link
Owner

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

Supportive - pending @mattsse

ethers-contract/src/call.rs Outdated Show resolved Hide resolved
Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>
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 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.

@gakonst gakonst merged commit 0236de8 into master Feb 6, 2023
@gakonst gakonst deleted the prestwich/borrow branch February 6, 2023 21:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants