CosmWasm / sylvia

CosmWasm smart contract framework
Apache License 2.0
96 stars 16 forks source link

Support generic interface on contract #237

Closed jawoznia closed 1 year ago

jawoznia commented 1 year ago

Part 1/2 of #226

This change lacks #[messages()] attribute on main #[contract] call as it is already a big PR

codecov[bot] commented 1 year ago

Codecov Report

Merging #237 (629f3a2) into feat/generics_support (eb685d4) will decrease coverage by 0.70%. The diff coverage is 79.83%.

@@                    Coverage Diff                    @@
##           feat/generics_support     #237      +/-   ##
=========================================================
- Coverage                  89.05%   88.36%   -0.70%     
=========================================================
  Files                         24       25       +1     
  Lines                       1380     1444      +64     
=========================================================
+ Hits                        1229     1276      +47     
- Misses                       151      168      +17     
Files Coverage Δ
sylvia-derive/src/input.rs 95.65% <100.00%> (+0.97%) :arrow_up:
sylvia-derive/src/multitest.rs 92.89% <100.00%> (+0.24%) :arrow_up:
sylvia-derive/src/utils.rs 69.69% <100.00%> (ø)
sylvia-derive/src/parser.rs 84.42% <80.00%> (-0.12%) :arrow_down:
sylvia-derive/src/check_generics.rs 89.28% <89.47%> (-5.16%) :arrow_down:
sylvia-derive/src/interfaces.rs 88.50% <65.00%> (-6.44%) :arrow_down:
sylvia-derive/src/message.rs 87.35% <74.07%> (-0.09%) :arrow_down:
sylvia/tests/generics.rs 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

hashedone commented 1 year ago

Also, what I noticed in the generated code (I cannot find it in the review, but it is important) - on dispatching _Phantom pattern, you make it unreachable!(). This doesn't seem right. Messages are external input, and you can get anything there. It can be custom JSON designed for this variant. Deserialization of this message is derived, so it is possible to happen. It should cause an error to be trackable.

Also - it is included in the schema, which needs to be corrected. We need to make PR to the schema generator, which adds syntax for ignoring the variant in the schema. Possible #[serde(skip)] would work here - it would also solve my previous comment, and unreachable! would be acceptable then (still, I think not preferable). But we need to ensure that schema properly interprets this #[serde(skip)], especially for queries.

hashedone commented 1 year ago

Yet another thing I am not sure where is generated - I see that generic types for messages are skipping generics not used in messages. It is messy if someone wants to build a message manually which might be sometimes needed - one would need to know which generics are to be passed. The idea of _Phantom variant here is to use generics that are not used in the particular message (eg for Cw1QueryMsg I imagine _Phantom variant to keep the Msg parameter). It is ok for it to keep all of them for simpler implementation.