Cw22 add supported interface by eledra89 · Pull Request #844 · CosmWasm/cw-plus

eledra89 · 2022-11-03T08:25:03Z

Implement #842

uint

Looks useful. Nice!

I left some comments. In general if we're doing a separate spec, I'd prefer to make it independent from cw2 rather than a superset.

peara · 2022-11-11T03:23:38Z

Hi @eledra89 @uint @ThienLK1. Since this is a new package, I think we could add more functionalities.
For example, instead of a Vec stored in a Item, we could use Map which map an interface_name to a version:
const SUPPORTED_INTERFACES: Map<&str, &str> = Map::new("supported_interfaces");
With that, we can support smart query like check_supported_interface(interface, version) which would make using this much more convenient.

eledra89 · 2022-11-11T03:35:29Z

also cc @shanev per our discussion on Twitter. Can't cc Larry here though

ThienLK1 · 2022-11-18T08:52:38Z

Hi @eledra89 @uint @ThienLK1. Since this is a new package, I think we could add more functionalities. For example, instead of a Vec stored in a Item, we could use Map which map an interface_name to a version: const SUPPORTED_INTERFACES: Map<&str, &str> = Map::new("supported_interfaces"); With that, we can support smart query like check_supported_interface(interface, version) which would make using this much more convenient.

@peara I just merge a new pull request, add more function to define and check version of supported interfaces, please take a look!

peara · 2022-11-21T06:55:47Z

+    store: &mut dyn Storage,
+    mut supported_interfaces: Vec<ContractSupportedInterface>,
+) -> Result<(), StdError> {
+    while let Some(supported_interface) = supported_interfaces.pop() {


can we use for supported_interface in supported_interfaces {} here?

yes, the code will be similar.

Argument change would force this for loop here, which is better anyway

peara · 2022-11-21T06:56:31Z

+        .unwrap_or_default();
+    let supported_version = Version::parse(&supported_version_rs);
+    match supported_version {
+        Ok(ver) => Ok(VersionResponse {


since this is a query, I think just return true or false would be enough.

it returns more information to the querier. Example which specifies version interface is supported, or not?

I think this function can be removed.

query_supported_interface_version already returns the version (or missing), be DRY.
Also, we will most likely be getting this though Map.query(...) as we remotely query a contract (don't compare with our own contracts version). Let's make the version comparison helper separate.

I would just add two functions that don't touch storage and show the semver usage, like:

// most comparisons just require a minimum - make this easy
minimum_version(version: &str, required: &str) -> bool {} - maybe make Result<(), Error>?

// allow more powerful query strings - ">=1.2.3,<2"
require_version(version: &str, request: &str) -> bool {} - or Result as above

If you go for the Result type, please define a custom Cw22Error type in the spec.

@ethanfrey for the minimum_version, you mean a helper function like this?

pub fn minimum_version(version: &str, required: &str) -> bool { let req = VersionReq::parse(">={required}").unwrap(); let version = Version::parse(version); req.matches(&version) }

eledra89 · 2022-11-28T22:55:48Z

@uint @ethanfrey , we added cw22: supported interface with additional support for version checking.

ethanfrey · 2022-11-29T10:37:42Z

Nice. I took a quick look over the comments and agree with the direction.

Using a separate independent interface, as well as using a Map for easier checks.

I want to say that one nice part of cw2 is that it has a well-defined storage structure, so you can do "raw queries" against it if needed. Making the storage layout part of this spec also sounds like a good idea. Sure, we can do smart queries, but that depends on a higher-level API and is less efficient. A well defined storage layer allows efficient raw queries with no more code on the queried contract.

We also have helpers for this, a calling contract can do something like:

let version = SUPPORTED_INTERFACES.query(contract_addr, "crates.io:cw20")?;

I will go through briefly on the code, but this is my high-level input.

ethanfrey

A number of comments on the code.

I like the direction, but please clean this up so it is up to standards to merge

ethanfrey · 2022-11-29T10:38:02Z

+* data: Json-serialized `ContractSupportedInterface`
+
+```rust
+pub struct ContractSupportedInterface {


Nit: this is different than implementation, please update

ethanfrey · 2022-11-29T10:38:32Z

+use query::VersionResponse;
+use semver::{Version, VersionReq};
+
+pub const SUPPORTED_INTERFACES: Map<String, String> = Map::new("supported_interfaces");


Please use Map<&str, String>, it is more efficient and easier to call

ethanfrey · 2022-11-29T10:39:37Z

+/// of supported interfaces. It should also be used after every migration.
+pub fn set_contract_supported_interface(
+    store: &mut dyn Storage,
+    mut supported_interfaces: Vec<ContractSupportedInterface>,


If we just use &str in the Map, this cheaply becomes: supported_interfaces: &[ContractSupportedInterface]

ethanfrey · 2022-11-29T10:40:06Z

+    store: &mut dyn Storage,
+    mut supported_interfaces: Vec<ContractSupportedInterface>,
+) -> Result<(), StdError> {
+    while let Some(supported_interface) = supported_interfaces.pop() {


Argument change would force this for loop here, which is better anyway

ethanfrey · 2022-11-29T10:42:19Z

+    mut supported_interfaces: Vec<ContractSupportedInterface>,
+) -> Result<(), StdError> {
+    while let Some(supported_interface) = supported_interfaces.pop() {
+        let id = supported_interface.supported_interface;


Why not just use a shorter local variable than "supported_interface" and inline this. The following should be clear enough (but change x to item or so if you prefer)

for x in supported_interfaces { SUPPORTED_INTERFACES.save(store, &x.supported_interface, &x.version)?; }

Actually, since we assume all stored versions are semver later on in queries, enforce it here.
Parse the values as Version and error if they are invalid - much better to error on instantiate (fail fast in tests) than later when first querying in production.

ethanfrey · 2022-11-29T10:43:39Z

+pub fn query_supported_interface_version(
+    store: &dyn Storage,
+    interface: String,
+) -> StdResult<ContractSupportedInterface> {


I would return StdResult<Option<String>> here. Unless there is a read error, it returns None for not registered, and the version if registered.

ethanfrey · 2022-11-29T10:44:07Z

+) -> StdResult<ContractSupportedInterface> {
+    let version = SUPPORTED_INTERFACES
+        .may_load(store, interface.clone())?
+        .unwrap_or_default();


We do want the difference between None and Some here (not just "")

ethanfrey · 2022-11-29T10:51:57Z

+        .unwrap_or_default();
+    let supported_version = Version::parse(&supported_version_rs);
+    match supported_version {
+        Ok(ver) => Ok(VersionResponse {


I think this function can be removed.

query_supported_interface_version already returns the version (or missing), be DRY.
Also, we will most likely be getting this though Map.query(...) as we remotely query a contract (don't compare with our own contracts version). Let's make the version comparison helper separate.

I would just add two functions that don't touch storage and show the semver usage, like:

// most comparisons just require a minimum - make this easy
minimum_version(version: &str, required: &str) -> bool {} - maybe make Result<(), Error>?

// allow more powerful query strings - ">=1.2.3,<2"
require_version(version: &str, request: &str) -> bool {} - or Result as above

ethanfrey · 2022-11-29T10:54:10Z

+use cosmwasm_schema::cw_serde;
+
+#[cw_serde]
+pub struct VersionResponse {


Let's remove this. I don't define smart queries for this, they require more wiring.

Let's be like cw2 (and cw4) and focus on spec'ing the storage layout for efficient queries. The contract should be able to easily write its own storage. Other contracts will be querying it's storage (Map.query) and testing the resulting value.

ethanfrey · 2022-11-29T10:54:36Z

+        .unwrap_or_default();
+    let supported_version = Version::parse(&supported_version_rs);
+    match supported_version {
+        Ok(ver) => Ok(VersionResponse {


If you go for the Result type, please define a custom Cw22Error type in the spec.

eledra89 · 2022-11-29T11:45:54Z

oh wow this is super detail @ethanfrey, we are kind of new to Rust & CosmWasm so this is very helpful in the long term !

shanev · 2023-06-04T23:47:25Z

Would love to get this merged in @eledra89 if you get a chance to resolve the PR comments.

peara · 2023-06-05T14:52:13Z

@shanev thanks for reminding us. We have been pretty busy. We will fix it this week.

ethanfrey · 2023-06-05T19:04:04Z

@uint no longer works at Confio and I am too booked to review this well.

@hashedone maybe you could recommend someone else who could review this? (Once they push the update. No rush, but it would be good to figure out who can review this)

Cw22 improvement

CLAassistant · 2023-06-07T07:19:43Z

All committers have signed the CLA.

hashedone · 2023-06-07T10:48:56Z

@ethanfrey I think I will just do this one myself for now.

hashedone

The idea is great, and the solution is pretty good. Still, I would strongly advise migrating from String to Cow - I know we do String in most places in cw, but we already tried to use Cow where possible in Sylvia. We should go for that in interfaces, particularly those which we want to be implemented by most of the contracts to reduce extra gas cost on those small things (I get it is only for instantiation/migration, but it is very much a free change).

@jawoznia please take a look at that - we probably will auto-support this in Sylvia in the future.

I give approval as the Cow thing is not super-critical, but if you can please align. If not, please at least create an issue out of my comment.

hashedone · 2023-06-07T10:59:44Z

+  /// e.g "crates.io:cw2"
+  /// NOTE: this is just a hint for the caller to adapt on how to interact with this contract.
+  /// There is no guarantee that the contract actually implement these interfaces.
+  pub supported_interface: String,


Nit:
This thing is mainly used with string literals to store it in DB. However, you always need to allocate the string for no real reason adding unnecessary cost. I understand you do it, so you can deserialize it keeping ownership.

You can avoid it with a Cow type. Just add a lifetime generic on top (ContractSupportedInterface<'a>) and then change all strings to Cow<'a, str>. Then instead of String::from("foo") you use Cow::borrowed("foo"), Cow::from("foo"), or even "foo".into(), and you avoid allocation unless you really need it (and you will not need it for serialization - serde handles (de)serialization through Cow transparently).

Ok, even though it is not exactly the same as in implementation, I would leave it this way - not sure if you forgot to update or left it on purpose, but it is probably easier to think about this way and it is semantically equivalent.

hashedone · 2023-06-07T11:00:30Z

+    supported_interface: String::from("crates.io:cw20"),
+    version: String::from("0.16.0"),


Yea, I may be a little bit biased but seeing this and then sending the whole structure by reference line later my whole brain screams "cost".

However, those should be updated - as supported_interface and version are Cow internally, this won't work. Both should be changed either to:

supported_interface: From::from("crates.io:cw20"), version: From::from("0.16.0"),

or

supported_interface: "crates.io:cw20".into(), version: "0.16.0".into(),

Both of those work for ContractSupportedInterface defined as String and as Cow. I personally prefer the .into() as it feels more natural to me, but I'll accept both.

Sorry for the annoying nitpicking here. We struggle with documentation quality, and we don't want to introduce any more dept in this area.

Suggested change

supported_interface: String::from("crates.io:cw20"),

version: String::from("0.16.0"),

supported_interface: "crates.io:cw20".into(),

version: String::from"0.16.0".into(),

hashedone · 2023-06-07T11:01:34Z

+/*!
+CW22 defines a way for a contract to declare which interfaces do the contract implement
+This standard is inspired by the EIP-165 from Ethereum. Originally it was proposed to
+be merged into CW2: Contract Info, then it is splitted to a separated cargo to keep CW2
+being backward compatible.
+
+Each supported interface contains a string value pointing to the corresponding cargo package
+and a specific release of the package. There is also a function to check whether the contract
+support a specific version of an interface or not.
+
+The version string for each interface follows Semantic Versioning standard. More info is in:
+https://docs.rs/semver/latest/semver/
+ */


Nit/consistency: we use /// for doc comments everywhere and for mod-level comments, //! - please stick to this pattern. It is more common in general in rust ecosystems.

hashedone · 2023-06-07T11:02:56Z

+    /// e.g "crates.io:cw2"
+    /// NOTE: this is just a hint for the caller to adapt on how to interact with this contract.
+    /// There is no guarantee that the contract actually implement these interfaces.
+    pub supported_interface: String,


This is where the comment about Cow should actually go.

peara · 2023-06-10T05:14:18Z

@hashedone I have updated with Cow.
I'd like to add some examples of QueryRaw like in cw4 but haven't been able to finished.
I also want to add this to cw20-base as well.
So maybe in a follow up PR, I will add this to cw20-base and add some QueryRaw examples as test there.

hashedone

LGTM with minor docs comment.

hashedone · 2023-06-10T08:51:01Z

+  /// e.g "crates.io:cw2"
+  /// NOTE: this is just a hint for the caller to adapt on how to interact with this contract.
+  /// There is no guarantee that the contract actually implement these interfaces.
+  pub supported_interface: String,


Ok, even though it is not exactly the same as in implementation, I would leave it this way - not sure if you forgot to update or left it on purpose, but it is probably easier to think about this way and it is semantically equivalent.

hashedone · 2023-06-10T08:56:53Z

+    supported_interface: String::from("crates.io:cw20"),
+    version: String::from("0.16.0"),


However, those should be updated - as supported_interface and version are Cow internally, this won't work. Both should be changed either to:

supported_interface: From::from("crates.io:cw20"), version: From::from("0.16.0"),

or

supported_interface: "crates.io:cw20".into(), version: "0.16.0".into(),

Both of those work for ContractSupportedInterface defined as String and as Cow. I personally prefer the .into() as it feels more natural to me, but I'll accept both.

Sorry for the annoying nitpicking here. We struggle with documentation quality, and we don't want to introduce any more dept in this area.

hashedone · 2023-06-10T08:57:33Z

+    supported_interface: String::from("crates.io:cw20"),
+    version: String::from("0.16.0"),


Suggested change

supported_interface: String::from("crates.io:cw20"),

version: String::from("0.16.0"),

supported_interface: "crates.io:cw20".into(),

version: String::from"0.16.0".into(),

peara · 2023-06-10T10:33:11Z

Sorry for the annoying nitpicking here. We struggle with documentation quality, and we don't want to introduce any more dept in this area.

@hashedone No problem, I actually forgot that, would have hate it myself.

ThienLK1 force-pushed the main branch from aa8ff79 to 884e17b Compare November 8, 2022 08:39

uint reviewed Nov 9, 2022

View reviewed changes

Comment thread packages/cw2-supported-interface/README.md Outdated

Comment thread packages/cw2-supported-interface/src/lib.rs Outdated

Comment thread packages/cw2-supported-interface/Cargo.toml Outdated

eledra89 marked this pull request as draft November 12, 2022 02:46

peara reviewed Nov 21, 2022

View reviewed changes

Comment thread packages/cw22/Cargo.toml Outdated

peara reviewed Nov 21, 2022

View reviewed changes

Comment thread packages/cw22/Cargo.toml Outdated

peara reviewed Nov 21, 2022

View reviewed changes

eledra89 marked this pull request as ready for review November 28, 2022 22:54

eledra89 changed the title ~~Cw2 add supported interface~~ Cw22 add supported interface Nov 28, 2022

ethanfrey suggested changes Nov 29, 2022

View reviewed changes

ThienLK1 and others added 11 commits June 7, 2023 11:28

add supported interface

ca3b764

update readme

cce9ca8

add cargo.lock

8ba749c

fix lint

55d2aca

add cw22

3d0f227

update README.md

5b4275d

Update README.md

a868481

add cw22 - define version of supported interface

719ff72

add cw22 - define version of supported interface

c774fb8

fix lint

80697a1

Updated config.yml

c3f5a41

ThienLK1 and others added 9 commits June 7, 2023 11:29

Updated config.yml

e42a428

remove unused dependencies

c072201

Add comments

af031e3

Fix lint

67a913c

add minimum_version & require_version

99ee321

add minimum_version & require_version

a07ac76

remove unused lib

df1f0c6

refactor minimum_version and require_version

c9e3508

update Cargo lock

43db6a0

peara force-pushed the main branch 2 times, most recently from 01d948c to de1fb0b Compare June 7, 2023 07:17

Merge pull request #11 from aura-nw/cw22_improvement

22c6aa6

Cw22 improvement

hashedone approved these changes Jun 7, 2023

View reviewed changes

peara added 2 commits June 8, 2023 01:04

Use Cow instead of String

a6c6106

chore: cargo fmt

1353e5b

hashedone approved these changes Jun 10, 2023

View reviewed changes

Update cw22 readme

fb37be3

		supported_interface: String::from("crates.io:cw20"),
		version: String::from("0.16.0"),

Conversation

eledra89 commented Nov 3, 2022

Uh oh!

uint left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

peara commented Nov 11, 2022

Uh oh!

eledra89 commented Nov 11, 2022

Uh oh!

ThienLK1 commented Nov 18, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eledra89 commented Nov 28, 2022

Uh oh!

ethanfrey commented Nov 29, 2022

Uh oh!

ethanfrey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eledra89 commented Nov 29, 2022

Uh oh!

shanev commented Jun 4, 2023

Uh oh!

peara commented Jun 5, 2023

Uh oh!

ethanfrey commented Jun 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hashedone commented Jun 7, 2023

Uh oh!

hashedone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ethanfrey commented Jun 5, 2023 •

edited

Loading

CLAassistant commented Jun 7, 2023 •

edited

Loading

peara commented Jun 10, 2023 •

edited

Loading