Extern ref types #586

ethanfrey · 2020-10-19T11:56:41Z

Closes #584

Adding types and showing the diff for hackatom contract.

It was just changing the function definitions, and then search replace in the test code: &mut deps -> deps.as_mut() and &deps -> deps.as_ref()

Update: After some nice conversations with an advanced Rust coder, I realized I could remove all the <S: Storage, A: Api, Q: Querier> generics in contract code if I just use eg. api: &dyn Api instead of api: &A. This definitely remove a lot of visual complexity from the contracts and makes them more approachable for Rust beginners.

This does involve some more changes to the contract code, but mainly removing complexity.

I would love some feedback when you can review. I think the newer contract code is much easier for devs starting with CosmWasm. And note that the vm code doesn't have to change at all, so this is backwards compatible with the wasm code generated from 0.11 (so we can stick with cosmwasm_version_4)

maurolacy · 2020-10-25T11:15:40Z

packages/std/src/deps.rs

@@ -22,3 +22,43 @@ impl<S: Storage, A: Api, Q: Querier> Deps<S, A, Q> {
        }
    }
 }
+
+pub struct DepsMut<'a, S: Storage, A: Api, Q: Querier> {
+    pub storage: &'a mut S,


Not sure I totally understand the problem (or the full space of the problem) but, if you want to provide discretionary mutable access to Storage, why not wrap Storage itself in a RefCell, i. e.:

pub storage: RefCell<&'a S>

That way, you can continue to use Deps as before (i.e. like &deps), and just borrow_mut() storage when mutable access is required.

I believe I would need RefCell<S> to be able to get a &mut S from &RefCell.

The cell can't give out something it never had

One reason motivating this (before removing generics) was that previously the caller had to own S and Q to be able to give a reference out. If the owner has &mut S and &Q that is all the access the handle function needs, but it could not actually call it. (I hit this trying to build a test framework).

Once I started using references, the idea of removing generics and using &dyn came up and got rid of a lot of noise

I believe I would need RefCell<S> to be able to get a &mut S from &RefCell.

I see, and you're right. And owning those objects is precisely what you're trying to avoid.

Kept thinking about this... I guess what I don't like (besides the multiplication of entities), is that this looks like a Rust "antipattern". I mean, in Rust the access mode (mut vs. const) is usually resolved by modifiers, or through a method call (as_mut() and such); not through object / struct definition.

Also, wouldn't make sense to just own Storage, or a copy of it? You can still have references for Querier and Api, but wouldn't make sense to pass Storage by value (or inside a RefCell if needed)?
I know this is against what you're trying to do here. Just pondering.

And, finally, maybe this can be resolved by passing a reference to a generic container, like Deps, but one that is not built on purpose, just to pass references, but is the original handler / container for those types. Not sure if this can be done given the current architecture / design decisions, though; and, if this wouldn't bring their own share of problems with multiple mutable access, borrowing, etc.

maurolacy

API looks good to me, and it's clearer/simpler than the previous one.

My only concern would be the general one I posted as a comment, i.e. if mutability selection could be restricted only to the storage member somehow.

webmaster128 · 2020-10-26T11:57:31Z

Haha, I see Mauro is stuggeling with this problem in a similar way I do. This concern is not about getting it compile, but this feeling that when things get super complicationd, you are probably doing something wrong. It feels like Rust is screeming don't try to code Java with me.

But looking at the facts, I think we can agree on the following:

A contract only needs a reference to a Storage and does not need to own it
For contract developers it is desireable to group Storage, Api and Queries into one thing.
There is no generics for mutability, such that handle and query need to receive different structs, one with a mutable and one with an immutable storage reference.

This at the end of the day make Ethan's solution the only one I see working. And I send a lot of time thinking about alternatives.

One change I'd like to propose (I can send a PR for the implementation) is to make the set of references the primary structure Deps: the "external dependencies" of a contract are a set of references. This gives us Deps/DepsMut as better names for the primariy structures (right now DepsRef is not a ref to Deps but something different). Owning dependencies is really a testing problem. In the production code we just have

fn _do_query<M, E>(/* … */) -> ContractResult<QueryResponse> {
    // ...
    let storage = ExternalStorage::new();
    let api = ExternalApi::new();
    let querier = ExternalQuerier::new();
    let deps = Deps {
        storage: &storage,
        api: &api,
        querier: &querier,
    };
    query_fn(deps, env, msg).into()
}

We then can have a cosmwasm_std::testing::OwnedDeps which is the result type of cosmwasm_std::testing::mock_dependencies.

Nice change using &dyn Storage instead of the generics.

webmaster128

A few things here. Biggest wish is the renaming (and change of perspective) as described in #586 (comment)

packages/std/src/deps.rs

packages/std/src/traits.rs

MIGRATING.md

webmaster128 · 2020-10-26T12:07:10Z

packages/std/src/traits.rs

+#[derive(Copy, Clone)]
+pub struct QuerierWrapper<'a> {
+    querier: &'a dyn Querier,
+}


Why did you create this extra type?

To solve a headache.

The Querier trait was implementing a lot of automatic functions (beyond the one we implement). These are generic functions. &dyn Querier as a trait objects does not support traits with generic functions (as there is not a fixed vtable with all possible trait functions). You can look in the history, there was a place where I pulled out S and A to &dyn Storage and &dyn Api, but left Querier. I had to add this type to get Querier to compile.

All those Querier "helper functions" can no longer be on the trait. Wrapping it in a struct seemed the easiest way to add it. But I guess using a newtype pub struct QuerierWrapper<'a>(&'a dyn Querer) is actually a clearer representation of how we wrap - like Binary/CanonicalAddr wrap Vec<u8>.

Secondly, we can adjust naming. Maybe QuerierWrapper=>Querier (as this is what contract devs will be seeing), and Querier => QuerierTrait or such, which is only used internally by core devs. Same way to make Deps, DepsRef and simplify the naming in the public API by giving the internals a more clunky naming.

I did the rename in a7efaa3. Happy to revert it if you don't like it, but I think this solves the issue with a cleaner public API.

maurolacy · 2020-10-26T13:20:24Z

Good analysis. I would like to challenge your point 3 above. There's a way to handle a reference with dynamically checked borrowing rules, and that's a RefCell.

Mutably borrowing from your code above, what I'm proposing is something along:

fn _do_query<M, E>(/* … */) -> ContractResult<QueryResponse> {
    // ...
    let storage = ExternalStorage::new();
    let api = ExternalApi::new();
    let querier = ExternalQuerier::new();
    let deps = Deps {
        storage: RefCell::new(storage),
        api: &api,
        querier: &querier,
    };
    query_fn(&deps, env, msg).into()
}

So, basically, passing a reference to Deps in query_fn() (instead of passing Deps by value), and mutably (and dynamically) borrowing Storage when needed.

This still copies Storage in RefCell::new(), but if the only Deps around is that one, which is passed by reference everywhere, that's OK.
You can even do something like

RefCell::new(ExternalStorage::new())

at initialization. Which is the logical thing to do, by the way.

ethanfrey · 2020-10-26T13:27:53Z

Mauro, I explicitly only want to pass in the minimal ownership needed - not move in storage (or even &mut Storage) if I will only be using &Storage. A lot of the practical type wrangling came from CosmWasm/cw-plus#130 which shows the issue with passing in owned values, and was my first draft on the DepsMut, DepsRef idea.

If I run recursive queries on a contract, that would be allowed in the Rust type system if they all have &Storage, but no more. I guess I could pass in &RefCell<Storage> and trust none of them try to get it mutable at runtime. But I also see &Storage as a much simpler API for contract developers than &RefCell<Storage>.

The point is this is not a normal struct, it really is just combining three args into one as they are commonly passed together. Simon did a spike to just pull them into 3 arguments and it made the API much messier. There has been quite some fiddling around to look for designs here, and supporting 2 different structs in the public API (Deps and DepsRef now) seems to be adding minimal complexity in order to save other issues.

maurolacy · 2020-10-26T14:13:54Z

But I also see &Storage as a much simpler API for contract developers than &RefCell<Storage>.

True. And, if the goal is precisley to make the API simpler, this is an important point.
Also, in terms of safety, what you say about restricting mutable access to static / compile time borrows makes sense.

I tend to forget we're dealing with an API here, and that it must not only work with the existing code base, but also be easy to use in future contracts / for future users.

webmaster128 · 2020-10-26T15:15:24Z

The issue with RefCell<Storage> is that it represents ownership of the storage. The contract implementation is then free to destroy the storage and never give it back to the caller. It can even disallow the caller to have a read-only reference.

webmaster128

Nice, we are getting there :)

Thanks for the explanation around QuerierWrapper. I'd revert a7efaa3. I think the old name was better, or at least more consistent with the rest. We'll find a way to nicelify it. We can use a newtype or a struct, no real preference. The newtype might be better to express the intend you explained.

webmaster128 · 2020-10-26T15:19:58Z

packages/std/src/deps.rs

+}
+
+#[derive(Copy, Clone)]
+pub struct DepsRef<'a> {


I'd call this Deps and the mutable version DepsMut. I think those names are better than the "Ref" suffix. This also uses the typical immutable default.

Okay, doing that now, then preparing for merge

packages/std/src/deps.rs

This reverts commit a7efaa3.

webmaster128

Alright, cool. Happy when CI is

webmaster128 · 2020-10-26T17:10:21Z

contracts/burner/src/contract.rs

@@ -1,12 +1,12 @@
 use cosmwasm_std::{
-    attr, Api, BankMsg, Binary, Deps, Env, HandleResponse, InitResponse, MessageInfo,
-    MigrateResponse, Order, Querier, StdError, StdResult, Storage,
+    attr, BankMsg, Binary, Deps, DepsMutMut, Env, HandleResponse, InitResponse, MessageInfo,


Suggested change

attr, BankMsg, Binary, Deps, DepsMutMut, Env, HandleResponse, InitResponse, MessageInfo,

attr, BankMsg, Binary, Deps, DepsMut, Env, HandleResponse, InitResponse, MessageInfo,

Yup, fixed this. And also rebuilt the test contract.

Getting ready to merge.

ethanfrey force-pushed the extern-ref-types branch from 68098dd to bdbf500 Compare October 24, 2020 21:54

ethanfrey marked this pull request as ready for review October 24, 2020 21:54

ethanfrey requested a review from webmaster128 October 24, 2020 21:56

ethanfrey added this to the 0.12.0 milestone Oct 24, 2020

ethanfrey requested a review from maurolacy October 25, 2020 09:41

maurolacy reviewed Oct 25, 2020

View reviewed changes

maurolacy approved these changes Oct 25, 2020

View reviewed changes

webmaster128 mentioned this pull request Oct 26, 2020

Improve naming and docs for &dyn ReadonlyStorage cast #593

Merged

webmaster128 reviewed Oct 26, 2020

View reviewed changes

webmaster128 mentioned this pull request Oct 26, 2020

Remove change_querier #595

Merged

ethanfrey and others added 15 commits October 26, 2020 15:22

Add ExternMut ExternRef types

4ddabec

Update hackatom with ExternRef ExternMut

0da50ec

Remove some generic type parameters

6bc3b80

Simplify hackatom contract

a9d33a1

Remove Q generic from ExternRef/Mut

097456b

Update entry points for hackatom, integration tests pass

287fc88

Port burner contract

2042c3e

Port queue

861bdd4

Start updating storage-bucket

6b0252d

Bucket works with dyn trait object

d4dc327

Singleton and Sequence work with trait objects

3f8ec8c

Update reflect contract

0e107e0

Fixed iterator support in bucket

a783c8a

Update staking contract

d638801

Improve naming and docs for &dyn ReadonlyStorage cast

90ea2b0

ethanfrey added 3 commits October 26, 2020 15:23

Format migrating.md

5a3f0d1

Rename Deps* types, add Copy to DepsRef

d0eed39

Update contracts for renaming

5e24955

ethanfrey force-pushed the extern-ref-types branch from 304fd60 to 5e24955 Compare October 26, 2020 14:25

Rename Querier types, for cleaner public Api

a7efaa3

webmaster128 reviewed Oct 26, 2020

View reviewed changes

ethanfrey added 2 commits October 26, 2020 17:31

Revert "Rename Querier types, for cleaner public Api"

414bccb

This reverts commit a7efaa3.

Deps/DepsRef -> DepsMut/Deps

b38e790

webmaster128 approved these changes Oct 26, 2020

View reviewed changes

Update vm test contract, fix typo

068cc37

webmaster128 added the automerge See mergify.io label Oct 26, 2020

mergify bot merged commit fe8557e into master Oct 26, 2020

mergify bot deleted the extern-ref-types branch October 26, 2020 17:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extern ref types #586

Extern ref types #586

ethanfrey commented Oct 19, 2020 •

edited

Loading

maurolacy Oct 25, 2020 •

edited

Loading

ethanfrey Oct 25, 2020 •

edited

Loading

ethanfrey Oct 25, 2020

maurolacy Oct 25, 2020

maurolacy Oct 26, 2020

maurolacy left a comment

webmaster128 commented Oct 26, 2020

webmaster128 left a comment •

edited

Loading

webmaster128 Oct 26, 2020

ethanfrey Oct 26, 2020

ethanfrey Oct 26, 2020

maurolacy commented Oct 26, 2020 •

edited

Loading

ethanfrey commented Oct 26, 2020

maurolacy commented Oct 26, 2020

webmaster128 commented Oct 26, 2020

webmaster128 left a comment

webmaster128 Oct 26, 2020

ethanfrey Oct 26, 2020

webmaster128 left a comment

webmaster128 Oct 26, 2020

ethanfrey Oct 26, 2020

	attr, BankMsg, Binary, Deps, DepsMutMut, Env, HandleResponse, InitResponse, MessageInfo,
	attr, BankMsg, Binary, Deps, DepsMut, Env, HandleResponse, InitResponse, MessageInfo,

Extern ref types #586

Extern ref types #586

Conversation

ethanfrey commented Oct 19, 2020 • edited Loading

maurolacy Oct 25, 2020 • edited Loading

Choose a reason for hiding this comment

ethanfrey Oct 25, 2020 • edited Loading

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

maurolacy left a comment

Choose a reason for hiding this comment

webmaster128 commented Oct 26, 2020

webmaster128 left a comment • edited Loading

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

maurolacy commented Oct 26, 2020 • edited Loading

ethanfrey commented Oct 26, 2020

maurolacy commented Oct 26, 2020

webmaster128 commented Oct 26, 2020

webmaster128 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webmaster128 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey commented Oct 19, 2020 •

edited

Loading

maurolacy Oct 25, 2020 •

edited

Loading

ethanfrey Oct 25, 2020 •

edited

Loading

webmaster128 left a comment •

edited

Loading

maurolacy commented Oct 26, 2020 •

edited

Loading