Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extension is difficult & other long term design decisions #31

Open
the-frey opened this issue Nov 26, 2021 · 5 comments · May be fixed by #127
Open

Extension is difficult & other long term design decisions #31

the-frey opened this issue Nov 26, 2021 · 5 comments · May be fixed by #127
Labels
discussion An issue that needs discussion help wanted Extra attention is needed question Further information is requested

Comments

@the-frey
Copy link
Collaborator

I think this is the starting point for a wider conversation. Or just tell me I'm imagining things. Either or.

FWIW I can use and extend this stuff fine now, hence why I will totally accept "nah, it's fine" as an answer.

However, I've found it a bit of an uphill struggle to get to grips with, and am still a little unclear on best practice for extending. I've seen several others taking similar or different approaches to this (on the query and the execute side). Generally, doing roughly what is in:

https://github.com/envoylabs/whoami/blob/main/src/lib.rs#L36 for execute and
https://github.com/CosmWasm/cw-nfts/blob/d79bd6f7803dd366430ade1b77edbeaea74946ca/contracts/cw2981-royalties/src/lib.rs#L79 for query

seems to be kinda how most folk do it (roughly).

I'm not experienced enough at Rust to make a perfect call on this, but my gut feeling is that the way CW721 base is implemented is:

  • unapproachable for newer rust developers
  • difficult to extend
  • results in copy-paste code in new subprojects
  • not actually saving much duplication

If I didn't have the Rust compiler to help, I'd be relatively lost.

I found myself just now needing to essentially copy-and-paste a big chunk of functionality, and I was thinking "how much of the base am I using and not overriding?" The way it works right now is as a container, and that's fine - because there is base behaviour in there that I am using, but I'm not sure what it is buying me over a module containing functions that I can import as-and-when-needed, if I'm having to implement new functions in a module and dispatch to them, instead of bolting stuff to the side of the container. The default implementations of query, execute etc I suppose?

What I fear is that base behaviour will change but not many contracts will pick it up, because it's being overriden, at which point you have to ask whether this should be a lego-bricks library of helper functions with some enums (closer to how some of the other cw-plus contracts are implemented, I suppose).

I'm sticking my neck out a little bit here, so feel free to say that I'm just not good with traits and I'm missing something, but... yeah. I feel like this is probably difficult to decide via e.g. discord and we need to have a design discussion around this. Even if it's just "the-frey, you're wrong, but X is an issue, let's talk about that."

Usage of these contracts is already exploding and soon we won't have the luxury to radically change base.

Which also brings into focus discussions like #25 - how we extend, what we consider e.g. base and plus and how we want to make a modular code base that is ergonomic to use while still being safe.

I don't think any of these are easy questions, and I certainly don't want to point fingers - everybody has worked hard to get this library to the place it is today (thanks!) but I see the window for foundational design closing, to some degree.

Maybe this also ties into the wider standards conversation that's happening. We're building the plane on the way down, and that's fine, but a few hours and some discussion now could save us some heartache in the future if we're going to be designing and building modular CW contracts for the next few years around NFTs.

Sorry about the essay and apologies if it's unclear what I'm trying to say. It's been a long day of code and possibly I need to revisit and refine this later.

@the-frey the-frey added help wanted Extra attention is needed question Further information is requested labels Nov 26, 2021
@the-frey
Copy link
Collaborator Author

I guess, to sum it up I'm questioning whether the approach of abstraction around contracts vs more of a pure library approach is a more maintainable pattern.

There's also the hybrid approach, where you build up a library of functions that take a context, say, that might be common things that nearly all hooks take - i.e. contract, depsmut, env for example.

It's not quite DI, but this kind of client/env/context injection is v common in large functional systems to have a singular representation of state (which is kind of what the current pattern does too, as it closes over that within the contract wrapper abstraction)

@Ruborcalor
Copy link

Ruborcalor commented Jan 2, 2022

Hey @the-frey thanks for opening this issue. Jumping on this a while after you posted but the difficulty of extension you pointed out is becoming more apparent and forcing developers to extend cw721 in non backwards compatible ways.

Talis for example updated the TokensResponse struct to be a vector of structs rather than a vector of strings (like in the base implementation) so that they could include more information like the metadata_uri in their implementation (example response).

Now when my discord bot interacts with nft contracts I have to do some validation to see which type of response I am getting back... https://github.com/GraviDAO/lunar-assistant-bot/blob/main/src/utils/terraHelpers.ts#L4

If a change is going to be made it would be better sooner rather than later like @the-frey said.

It would be nice to follow the approach of the Open-closed principle which states that "software entities (classes, modules, functions, etc.) should be open for extension, but closed for modification". In the case of TokensResponse this could mean making the base version a vector of structs which just define the token id. This would make it so that Talis and others could extend from it in a backwards compatible way.

@JakeHartnell
Copy link
Collaborator

Think #42 goes a long way towards closing this.

@the-frey
Copy link
Collaborator Author

I think 42 digs us out of the hole we're currently in with extensions (and is a great piece of work given where we were), but does nothing to address the core issue this was opened to discuss (for my 2cs, anyway). I think there's actually also an overlap here with the authz discussion that was had re: x/wasm that essentially reveals there is:

  1. an over-opinionated pattern in code to extend the contracts
  2. an adequately opinionated pattern as best practice in contracts to expose an API/interface
  3. a completely unopinionated API from x/wasm

These things are where the tension lies, I think.

@JakeHartnell JakeHartnell added the discussion An issue that needs discussion label Jul 25, 2022
@JakeHartnell
Copy link
Collaborator

The more I look at the great work @ethanfrey and the Confio folks are doing on the latest mesh-security contracts with Sylvia, the more I'm starting to think that may be our best path forward. Would be a substantial re-write but could also make extension much easier without having to use traits.

Would mean some breaking changes, so not to be taken on lightly, but could be promising? Additionally, may also be worth exploring leveraging the new Cosmos SDK NFT module for much the same reasons as Token Factory (though this is a separate discussion).

@JakeHartnell JakeHartnell linked a pull request Jun 21, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion An issue that needs discussion help wanted Extra attention is needed question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants