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

Support generic interface on contract #237

Merged

Conversation

jawoznia
Copy link
Collaborator

@jawoznia jawoznia commented Oct 6, 2023

Part 1/2 of #226

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

@jawoznia jawoznia changed the base branch from main to feat/generics_support October 6, 2023 13:58
@jawoznia jawoznia force-pushed the support_generic_interface_on_contract branch from 57f4389 to 8470f7f Compare October 6, 2023 14:19
@jawoznia jawoznia marked this pull request as ready for review October 6, 2023 14:20
@jawoznia jawoznia requested a review from hashedone October 6, 2023 14:20
@jawoznia jawoznia force-pushed the support_generic_interface_on_contract branch from 8470f7f to 9a0ac19 Compare October 10, 2023 11:27
@codecov
Copy link

codecov bot commented Oct 10, 2023

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%) ⬆️
sylvia-derive/src/multitest.rs 92.89% <100.00%> (+0.24%) ⬆️
sylvia-derive/src/utils.rs 69.69% <100.00%> (ø)
sylvia-derive/src/parser.rs 84.42% <80.00%> (-0.12%) ⬇️
sylvia-derive/src/check_generics.rs 89.28% <89.47%> (-5.16%) ⬇️
sylvia-derive/src/interfaces.rs 88.50% <65.00%> (-6.44%) ⬇️
sylvia-derive/src/message.rs 87.35% <74.07%> (-0.09%) ⬇️
sylvia/tests/generics.rs 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

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

@jawoznia jawoznia force-pushed the support_generic_interface_on_contract branch from 9a0ac19 to 08d5381 Compare October 11, 2023 12:53
Copy link
Collaborator

@hashedone hashedone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks okish, but I really struggle with this AsIdent and dropping the entire path for generics verification - it seems to assume that if paths have the same ident, they are the same, which is not the case. We agreed on the call you would double-check this.

The tests that are the most important look good - I'll check the code generated there as well, but I need to understand what happens with those paths first.

sylvia-derive/src/check_generics.rs Outdated Show resolved Hide resolved
sylvia-derive/src/check_generics.rs Outdated Show resolved Hide resolved
@hashedone
Copy link
Collaborator

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
Copy link
Collaborator

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.

@jawoznia jawoznia force-pushed the support_generic_interface_on_contract branch 2 times, most recently from 50dce2d to 6dc3bc6 Compare October 18, 2023 07:19
Copy link
Collaborator

@hashedone hashedone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with one test request.

sylvia/tests/generics.rs Outdated Show resolved Hide resolved
@jawoznia jawoznia force-pushed the support_generic_interface_on_contract branch from 6dc3bc6 to 629f3a2 Compare October 18, 2023 10:39
@jawoznia jawoznia merged commit f046d9e into feat/generics_support Oct 18, 2023
5 of 7 checks passed
@jawoznia jawoznia deleted the support_generic_interface_on_contract branch October 18, 2023 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow implementing generic interface on contract
2 participants