Make NFT Customisable #445

ethanfrey · 2021-09-21T09:08:38Z

Closes #440

turn cw721 contract into a struct
make it generic over all Response types
tests pass
add generic extension to the state
expose generic extensions in query

This is also a step to make a more reusable contract structure, looking more like controllers and the ideas in #391
We can iterate on this (what belongs in lib.rs, how to customise the generics, etc), but I think this is a big step to re-usability. Maybe best to get feedback from people trying to extend this.

hashedone · 2021-09-21T11:41:30Z

contracts/cw721-base/src/contract.rs

+    pub token_count: Item<'a, u64>,
+    /// Stored as (granter, operator) giving operator full control over granter's account
+    pub operators: Map<'a, (&'a Addr, &'a Addr), Expiration>,
+    pub tokens: IndexedMap<'a, &'a str, TokenInfo<T>, TokenIndexes<'a, T>>,


I like the idea of having state as part of implementation structure as a part of contract type itself - I didn't put it in the original proposal as I wanted to make it minimal, but if it is achievable then I think it is a good idea.

It's not just achievable, it is essential in this use-case to allow the state itself to be generic over T and ensure all state access uses the same T

hashedone · 2021-09-21T11:42:27Z

contracts/cw721-base/src/contract.rs

+    pub operators: Map<'a, (&'a Addr, &'a Addr), Expiration>,
+    pub tokens: IndexedMap<'a, &'a str, TokenInfo<T>, TokenIndexes<'a, T>>,
+
+    _custom_response: PhantomData<C>,


There is no reason to make this contract generic over C. If contract interface would be a trait, it should be an associated type (as for any type T there is only one well defined type C).

Good point. Each method (execute_*) should then just take C as a generic parameter for this method, right?

I didn't think of it, but also part of it comes from laziness.

C: Clone + std::fmt::Debug + PartialEq + JsonSchema is a mouthful to put everywhere.

I would like to do something like type CustomMsg = Clone + std::fmt::Debug + PartialEq + JsonSchema to make this simpler, but last time I tried something similar, I got compiler errors. I could make a trait:

pub trait CustomMsg: Clone + std::fmt::Debug + PartialEq + JsonSchema {}

But that would require me to also implement the trait specifically for all messages (which are very few, so many that is actually a good idea)

@hashedone I addressed this in b32b21e

I agree it is more proper. I also made the CustomMsg trait as a placeholder (but we need to move it somewhere else to really use it).

One thing I noticed is it made tests a bit more verbose (adding ::<Empty> in lots of places), but this is the cost of generics I guess.

Each method (execute_*) should then just take C as a generic parameter for this method, right?

No, C should be a part of a trait defining an interface of this contract as an associated type, and it would be defined as a trait implementation. You can think that the trait performs as a functor which defines C in terms of T. Obviously to make it happen you need to have this kind of functor, I don't think it is achievable without extracting implementation to trait.

The issue is that I want my Cw721BaseContract struct to implement Cw721<C, T> for any C and T.
There is no correlation between the two. C is usually Empty, but if you need to integrate with Custom blockchain messages it is possible. T may be many values based on who is extending it.

If I do something like:

trait Cw721<T> where T: Serialize + DeserializeOwned + Clone { type Err: ToString; type C: Serialize + DeserializeOwned + Clone; }

like you do below, it hardcodes the Response and only allows 1 C for a given contract. This does now allow us to extend it for CustomMsg.

I agree fully with type Err, as that is fixed in the implementation (but not by the trait). But I would move C up to a trait generic rather than an associated type (not sure the proper terms here... please correct me, but I think you got the idea)

I am looking at this and I don't get the usecase, but it would probably be more clear when I would see it. I just cannot imagine a case when you want more than one implementation of a trait per contract. Now I am even thinking that even T is probably the associated type, as at the end for your entry points you have to make proper type for deserialization, and at the end your contract is basically setting it in stone. On the other hand it may be, that the contract itself is operating on very specific T/C, but maybe somewhere in future when you extend it you may pass-forward calls and then it is useable. But I don't see generic usage in C in the implementation so I am not sure.

~~Anyway - let leave it for now, it is huge step forward anyway, and I think if we will observe how it evolves, we would have better POV on this.~~

Forget it - right after comiting this comment I figured out you are actually using C everywhere as Response generic param, and it is critical for forwarding calls in extending contracts, and I had this problem before with non-generic controllers. It turns out sometimes you need to be wrong and write your bullshit to understand the point you are missing (where you is me in this case).

Anyway - I am pretty sure that you need this C generic to be trait parameter, but you don't need this generic for the structure itself (and also you don't need a phantom data), you can just implement your trait for contract like:

impl<T, C> Cw721<T, C> for Cw721Contract<T> { // ... }

But for now leave it as it is, as removing the generic from struct is possible only when logic is in trait, and it is like next step.

hashedone · 2021-09-21T11:51:04Z

contracts/cw721-base/src/contract.rs

-        .add_attribute("recipient", contract)
-        .add_attribute("token_id", token_id))
-}
+    pub fn execute(


I would definitely move this out of this struct implementation, probably to the structure message, and make it dispatching over any contract which implements a trait representing the interface of this contract. In this particular case, the trait would be a Cw721, and the execute would be:

impl<T> ExecMsg<T> { /// Dispatches this message to contract fn execute<Contract: Cw721>( self, contract: &Contract, deps: DepsMut, env: Env, info: MessageInfo ) -> Result<Response<Contract::C>, Contract::Err> { match self { ExecuteMsg::Mint(msg) => contract.mint(deps, env, info, msg), // All other branches } } }

Err and C (Which should get some meaningfull name - there is no restriction on generic arguments identifier lengths ;) ) are dependent on actual contract implementation (as it may and possibly would differ for different contracts).

This makes sense.

The problem is this is also not reusable if they add even one field to the ExecMsg in some extension. They would need something like impl TryFrom<CustomExecMsg> for ExecMsg, which would be quite elegant if we could use #[serde(flatten)] but we cannot do that yet.

I could do this inversion of control (no problem), but it is only really useful if all derived contracts have the exact same message structure or we can use flatten to extend them.

It still can be done bypassing serde (implementing Serialize just by forwarding calls to the enums), but it looks like much to be done (except if it may be generated in which case it is free). The clue of #391 is for "extending" message not to contain elements from message to be extended and dispatch them level higher.

hashedone · 2021-09-21T11:58:36Z

contracts/cw721-base/src/contract.rs

-    tokens().save(deps.storage, &token_id, &token)?;
-    Ok(token)
-}
+    pub fn execute_mint(


Now there is the clue of #391 - the idea is to move all of those functions to the trait like:

trait Cw721<T> where T: Serialize + DeserializeOwned + Clone { type Err: ToString; type C: Serialize + DeserializeOwned + Clone; fn mint( &self, deps: DepsMut, _env: Env, MessageInfo, msg: MingMsg<T> ) -> Result<Response<Self::C>, Self::Err>; }

Then the trait is implemented by contract itself, but may also be implemented by any other contract which also react to this contract msg.

Also I find this naming very verbose - basically execute sounds like do action so it feels like mint is just expressive enough. I would personally avoid prefixing functions if the name is not ambiguous, and leave them matching the message name - but this is side note.

Yes, I was doing the minimal subset here. I will pull out traits, sounds good.

type C: Serialize + DeserializeOwned + Clone

Note this contradicts what I just did in b32b21e which makes C generic on the method (you fix it for the contract when implementing the trait).

If we remove Self::C and keep Self::Error I can see this coming along

I started a bit of this in 19803f2 to get first feedback.

Note that Mint is not in the generic cw721 standard, so we have the case already of the contract extending the default. This is more when you try to move execute into the message type.

I actually don't see any sense in making C generic in methods - its semantic is to be always the same for given T. it makes no sense for having it generic to be decided on call moment, and more important - there is no real way to elide its type there!

Let's discuss this in person. I think this demo makes the issues clear at least, so we can discuss the design with a concrete case

I actually don't see any sense in making C generic in methods - its semantic is to be always the same for given T. it makes no sense for having it generic to be decided on call moment, and more important - there is no real way to elide its type there!

T is the extra data added to TokenInfo when storing it.
C is custom message type from Response.
These have nothing to do with each other.

But I do agree to putting this back into a trait/struct bound not a method bound

Yup, I figured it out bit later. It should definitely be on trait bound, probably not necessary for a struct itself (you can implement generic struct with generics which your struct is not generic over), but it is a detail (I elaborated in different comment).

hashedone · 2021-09-21T11:58:58Z

contracts/cw721-base/src/contract.rs

-        } => to_binary(&query_tokens(deps, owner, start_after, limit)?),
-        QueryMsg::AllTokens { start_after, limit } => {
-            to_binary(&query_all_tokens(deps, start_after, limit)?)
+    pub fn query(&self, deps: Deps, env: Env, msg: QueryMsg) -> StdResult<Binary> {


Same as for execute

hashedone

Basically like it, but I find very important point of #391 missing - extract interfacing of contract to a trait instead of purely message (and leaving message only to be the [de]serialization and dispatching point). Not approving nor requesting changes at this point, as it is more like to be discussed stuff.

hashedone · 2021-09-21T12:04:42Z

contracts/cw721-base/src/contract.rs

+    T: Serialize + DeserializeOwned + Clone,
+    C: Clone + std::fmt::Debug + PartialEq + JsonSchema,
+{
+    pub fn instantiate(


In contradiction to execute/query this should stay in contract itself (as it is very much per contract stuff).

contracts/cw721-base/src/contract.rs

hashedone · 2021-09-21T12:07:19Z

contracts/cw721-base/src/lib.rs

+        msg: msg::ExecuteMsg<Extension>,
+    ) -> Result<Response, ContractError> {
+        let tract = contract::Cw721Contract::<Extension, Empty>::default();
+        tract.execute(deps, env, info, msg)


Relating to other comment about moving this to message itself it would obviously go into form of msg.execute(contract, deps, env, info), and similar for query.

ethanfrey · 2021-09-21T12:27:16Z

Thanks for the feedback. Some stuff I will implement right away, other we can discuss more.
Let me update this for the places we agree then we discuss the remainders

hashedone · 2021-09-21T14:24:14Z

contracts/cw721-base/src/lib.rs

+        env: Env,
+        info: MessageInfo,
+        msg: msg::ExecuteMsg<Extension>,
+    ) -> Result<Response, ContractError> {


I was thinking about the problem of handling multiple types of messages (like base message and extension message), and I got a revelation - this is a place. As it is just a boilerplate it makes sense to instead of take msg as msg::ExecuteMsg<Extension>, take it here as something generic, before it is parsed (like Binary), and then just make deserialization here, like (this is more pseudocode):

let contract = contract::Cw721Contract::default(); if let Ok(msg) = deserialize::<msg::ExecuteMsg<Extension>>() { Cw721::execute(&contract, deps, env, info, msg) } else if let Ok(msg) = deserialize::<extension::ExecuteMsg>() { Extension::execute(&contract, deps, env, info, msg) } else { return Err(StdError::InvalidMessage.into()); }

The only problem I have here is if there is any type which is cheap/free to deserialize into by entry_point (so deserialization is not performed twice). In future, when this is generated it can even generate raw entry point.

I would auto-generate some ser/deserialization code that does that.

eg.

enum ExtendedExecMsg { Cw721(cw721::ExecMsg), My(crate::ExecMsg), }

And auto-generate ser/deser such that it flattens them, basically doing what you do above - try to deter into cw721::ExecMsg, if that fails, deser into My.

This is a subset of the serde "flatten" option, and maybe you want to make a macro to implement Serialize on this? Or maybe we try it first with a manually implemented Serialize/Deserialize and see if it makes sense.

JakeHartnell · 2021-09-22T01:49:34Z

contracts/cw721-base/src/state.rs

@@ -18,6 +19,9 @@ pub struct TokenInfo {
    pub description: String,
    /// A URI pointing to an image representing the asset
    pub image: Option<String>,
+
+    /// You can add any custom metadata here when you extend cw721-base
+    pub extension: T,


This is great! Not sure if description / image are needed anymore if there is an extension?

Much more usable like this though. 🙏

This reverts commit b32b21e.

ethanfrey · 2021-09-22T20:03:10Z

@hashedone I think I have done all I will do in this PR.

Outstanding questions:

Do you agree with my use of C as a trait generic?
What do you think of the two smaller interfaces (Cw721Query, Cw721Execute) which can be implemented separately?
- And how to deal with associated types like Err?
Should we implement and export a Cw721Base trait for the extra calls we have (mint and minter) that extends Cw721?
Separating query.rs and execute.rs? Good idea or bad? (The 1300+ line contract.rs was already giving me headaches scrolling up and down rather than switching files, so I prefer to separate the tests out at least)

The issue with dispatching in execute and query is a good one. I think putting that in the Msg types makes sense as soon as we can easily merge multiple enums (even manually implementing Serialize/Deserialize). Happy for a follow-up PR if you want to do that, then pull most of the dispatching into cw721::ExecuteMsg / cw721::QueryMsg. That is too big to pile into an already enormous PR.

hashedone

Nice refactor, I like it at this point.

ethanfrey added 7 commits September 21, 2021 10:43

Move state objects into a struct

8e8dc8e

Move instantiate into contract struct

faddb1c

Move execute into struct

06c1bb5

Compiles with new structure

77948f1

Fix tests

4a25d25

Add customizable extension to ContractInfo

45d8212

Expose extension: T in NftInfo queries

406a711

ethanfrey marked this pull request as ready for review September 21, 2021 09:47

ethanfrey requested a review from hashedone September 21, 2021 09:51

hashedone reviewed Sep 21, 2021

View reviewed changes

contracts/cw721-base/src/contract.rs Outdated Show resolved Hide resolved

hashedone reviewed Sep 21, 2021

View reviewed changes

ethanfrey added 2 commits September 21, 2021 14:50

Moved C generic into execute* calls not struct

b32b21e

Start on the Cw721 trait, with one method, to demo

19803f2

hashedone reviewed Sep 21, 2021

View reviewed changes

JakeHartnell reviewed Sep 22, 2021

View reviewed changes

ethanfrey added 5 commits September 22, 2021 21:14

Revert "Moved C generic into execute* calls not struct"

71eeea5

This reverts commit b32b21e.

Execute methods pulled into Cw721 trait

6b55583

Pull all queries into traits

b81dbb8

Pull Cw721Contract struct and Maps into state.rs

271a733

Experimental: split contract into execute, query, contract_tests

51f9cb9

hashedone approved these changes Sep 23, 2021

View reviewed changes

ethanfrey merged commit 9671cc2 into 0.9.x Sep 23, 2021

ethanfrey deleted the 440-customizable-nft branch September 23, 2021 07:11

This was referenced Sep 23, 2021

Demo metadata extension #450

Merged

Allow NFTs to include custom data #440

Closed

Forward port 440-customize-nft #451

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make NFT Customisable #445

Make NFT Customisable #445

ethanfrey commented Sep 21, 2021 •

edited

hashedone Sep 21, 2021

ethanfrey Sep 21, 2021

hashedone Sep 21, 2021

ethanfrey Sep 21, 2021

ethanfrey Sep 21, 2021

hashedone Sep 21, 2021

ethanfrey Sep 22, 2021 •

edited

hashedone Sep 23, 2021 •

edited

hashedone Sep 21, 2021 •

edited

ethanfrey Sep 21, 2021

hashedone Sep 21, 2021 •

edited

hashedone Sep 21, 2021 •

edited

ethanfrey Sep 21, 2021

ethanfrey Sep 21, 2021

ethanfrey Sep 21, 2021

hashedone Sep 21, 2021

ethanfrey Sep 21, 2021

ethanfrey Sep 22, 2021

hashedone Sep 23, 2021

hashedone Sep 21, 2021

hashedone left a comment •

edited

hashedone Sep 21, 2021

hashedone Sep 21, 2021

ethanfrey commented Sep 21, 2021

hashedone Sep 21, 2021

ethanfrey Sep 21, 2021

JakeHartnell Sep 22, 2021

ethanfrey commented Sep 22, 2021

hashedone left a comment

Make NFT Customisable #445

Make NFT Customisable #445

Conversation

ethanfrey commented Sep 21, 2021 • edited

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

ethanfrey Sep 22, 2021 • edited

Choose a reason for hiding this comment

hashedone Sep 23, 2021 • edited

Choose a reason for hiding this comment

hashedone Sep 21, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashedone Sep 21, 2021 • edited

Choose a reason for hiding this comment

hashedone Sep 21, 2021 • edited

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

hashedone left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey commented Sep 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey commented Sep 22, 2021

hashedone left a comment

Choose a reason for hiding this comment

ethanfrey commented Sep 21, 2021 •

edited

ethanfrey Sep 22, 2021 •

edited

hashedone Sep 23, 2021 •

edited

hashedone Sep 21, 2021 •

edited

hashedone Sep 21, 2021 •

edited

hashedone Sep 21, 2021 •

edited

hashedone left a comment •

edited