CosmWasm / cosmwasm

Framework for building smart contracts in Wasm for the Cosmos SDK
https://www.cosmwasm.com/
Apache License 2.0
1.06k stars 329 forks source link

Convention: Negate "library" feature? #1627

Open webmaster128 opened 1 year ago

webmaster128 commented 1 year ago

This is not really a topic of this repository but a larger CosmWasm ecosystem convention. cw-plus introduced a "library" feature convention to be able to use one contract package as a dependency of another contract. As brought up by multiple community members[^1] ^2, the library feature used to disable entry points is not additive.

A consequence of this is that features should be additive. That is, enabling a feature should not disable functionality, and it should usually be safe to enable any combination of features. A feature should not introduce a SemVer-incompatible change.

https://doc.rust-lang.org/cargo/reference/features.html

While I think in general this is the right™ way, there are a few things that are no clear to me yet:

[^2]: Who can help me find that earlier Twitter conversation that also included Shame and others?

hashedone commented 1 year ago

I was thinking about it a long time ago, but I remember there were some problems with that - I don't remember exactly, but I think something with the dependency resolver. I would vote strongly "for" this feature. I would start with picking up some repo with more than one, but not too many contracts, try to apply the change there and then see if there are any issues (I can easily be wrong about that, I remember I tried to do something around that over a year ago so maybe I did something stupid, and that was causing problems).

aeryz commented 1 year ago

Would this make adding a contract as a library much harder because you cannot unselect a single feature in Rust?

Assuming there will be an entry_points feature for exporting the entry points, if a contract would want to use a cw-plus contract as a dependency, it needs to do:

cw20-base = { version = "*", default-features = false, features = [ /* remaining default features */ ] }

instead of:

cw20-base = { version = "*", features = [ "library"] }

I don't think it makes it much harder because currently there is no additional default features that is needed to be included, the only default feature will be entry_points. Even if you add default features, it can still be solved with a single line of documentation I guess.

Would a negation achieve the additive property? Adding those global C exports probably still conflicts with it (imagine packages libraries with the entry_points feature enabled).

Do you mean whether this can cause any problem if a dependency depends on another package with entry_points feature enabled? eg.

A depends on:
-------> B (default-features = false) depends on:
        ----------> C (entry_points is enabled, hence A's exports conflict with C)

Shouldn't a contract and a library be in separate packages or build targets? A native executable (bin or example) is also built separately from the lib code. The idea of depending on a contract feels non-idiomatic.

I am also interested in seeing the use case here (how the failure happened).

ethanfrey commented 1 year ago

Shouldn't a contract and a library be in separate packages or build targets? A native executable (bin or example) is also built separately from the lib code. The idea of depending on a contract feels non-idiomatic.

Actually I think this may be an interesting approach. All importable code in src and some minor shim in bin/wasm.rs or whatever to provide the exports. Not sure how doable this is or if it makes sense, but it would make sure that no one importing the source code could ever activate the external "C" exports (which is what we want to achieve with all this)

aumetra commented 1 week ago

We can detect whether the current compilation unit (i.e. crate) is a dependency or not. Cargo sets the environment variable CARGO_PRIMARY_PACKAGE not for dependencies, so this code should detect it:

option_env!("CARGO_PRIMARY_PACKAGE").is_none()