feat: Add InstantiateBuilder and SubMsg trait #437

jawoznia · 2024-09-26T13:27:58Z

No description provided.

codecov · 2024-09-26T13:32:42Z

Codecov Report

Attention: Patch coverage is 89.26554% with 19 lines in your changes missing coverage. Please review.

Project coverage is 72.20%. Comparing base (72ebc77) to head (8747605).
Report is 3 commits behind head on feat/replies.

Files with missing lines	Patch %	Lines
sylvia/src/builder/instantiate.rs	47.82%	12 Missing ⚠️
sylvia/tests/reply.rs	86.48%	5 Missing ⚠️
sylvia-derive/src/contract.rs	93.33%	1 Missing ⚠️
sylvia-derive/src/contract/communication/reply.rs	98.55%	1 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff                @@
##           feat/replies     #437      +/-   ##
================================================
+ Coverage         71.23%   72.20%   +0.96%     
================================================
  Files                54       56       +2     
  Lines              3299     3457     +158     
================================================
+ Hits               2350     2496     +146     
- Misses              949      961      +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

hashedone

In general I like the API, but I don't like that the tests are just expanded to generics to cover more cases. I'd prefer to have separated tests for simple things, and for generics. This way tests serves as better examples, and also if something breaks it is more clear where it breaks.

hashedone · 2024-10-02T16:43:50Z

sylvia/tests/reply.rs

    use sylvia::{contract, entry_points};

    use sylvia::cw_std::Response;

-    pub struct NoopContract;
+    pub struct NoopContract<M, Q> {
+        _phantom: std::marker::PhantomData<(M, Q)>,


Do we need to extend this contract to be generic? Tests often serves as examples, I think it would be good to maintain simple contracts testing basics, and then separate generic/more complex contract testing the more specific things.

We could, however this would more or less lead to duplication of these tests and contracts.

Also we decided that using generic for custom message and custom query is a good practice.

kulikthebird

Lgtm. The only thing that we discussed are the replies after instantiation procedure. Something like callback_{name_of_the_method}

kulikthebird · 2024-10-08T10:40:11Z

sylvia-derive/src/contract/communication/reply.rs

@@ -266,12 +366,13 @@ impl<'a> ReplyVariant<'a> for MsgVariant<'a> {
        self.msg_attr().handlers().iter().collect()
    }

-    fn as_reply_data(&self) -> Vec<(Ident, &Ident, ReplyOn)> {
+    fn as_reply_data(&self) -> Vec<(Ident, &Ident, &Ident, ReplyOn)> {


Let's have a struct instead of tuple in the result of this method

kulikthebird · 2024-10-08T10:40:15Z

sylvia-derive/src/contract/communication/reply.rs

@@ -255,7 +355,7 @@ fn emit_failure_match_arm(

 trait ReplyVariant<'a> {
    fn as_handlers(&'a self) -> Vec<&'a Ident>;
-    fn as_reply_data(&self) -> Vec<(Ident, &Ident, ReplyOn)>;
+    fn as_reply_data(&self) -> Vec<(Ident, &Ident, &Ident, ReplyOn)>;


Let's have a struct instead of tuple in the result of this method

I didn't want to create an intermediate struct for that, but I will just move this logic to the as_reply_data method.

If it's okay with you I would do that in the latest PR to avoid conflicts.

kulikthebird · 2024-10-08T10:44:53Z

sylvia-derive/src/contract/communication/reply.rs

+        } else if is_failure {
+            quote! { #sylvia ::cw_std::ReplyOn::Error }
+        } else {
+            // This should never happen


Why this should never happen? If so, the code itself should prevent such a path (and raise an error in compilation time to the user)

It will earlier while parsing. We support Success, Failure, and Always, as specifying logic that would Never be called makes no sense.

I will add better comment there.

kulikthebird · 2024-10-08T10:46:19Z

sylvia-derive/src/contract/communication/reply.rs

@@ -146,28 +216,31 @@ impl<'a> ReplyVariants<'a> for MsgVariants<'a, GenericParam> {
 /// Maps single reply id with its handlers.
 struct ReplyData<'a> {
    pub reply_id: Ident,
+    pub handler_name: &'a Ident,


What's the difference between handler_name and handlers? Both names suggest that they are somehow related to each other

I will figure out some better naming.

The handler_name is either method name or value from handlers parameter. The reply_id is constructed based on it.

The handlers are pairs of method names and reply_on values to determine what logic in which scenario should be called.

kulikthebird · 2024-10-09T11:00:45Z

sylvia/tests/reply.rs

        self.last_reply.save(ctx.deps.storage, &ALWAYS_REPLY_ID)?;

        Ok(Response::new())
    }
+
+    #[sv::msg(exec)]
+    fn send_cosmos_messages(&self, ctx: ExecCtx<Q>) -> Result<Response<M>, ContractError> {


Do we use this method?

Good catch. I missed to call it in the test.

uint

LGTM overall. A neat feature is delivered. I don't see anything harmful, so approving 👍

uint · 2024-10-09T11:52:06Z

sylvia-derive/src/contract/communication/instantiate_builder.rs

+use crate::types::msg_variant::MsgVariant;
+use crate::utils::{get_ident_from_type, SvCasing};
+
+pub struct InstantiateBuilder<'a> {


Could this InstantiateBuilder::new(...).emit() dance be replaced by a single emit_instantiate_builder function?

It might. This is just how all the code looks like in the sylvia-derive.
The benefit is that in some cases we want to do some validation and data preparation in the new method which makes the emit method more readable.

uint · 2024-10-09T11:55:23Z

sylvia-derive/src/contract/communication/instantiate_builder.rs

+                fn #method_name < #(#used_generics),* > (code_id: u64, #(#parameters,)* ) -> #sylvia ::cw_std::StdResult< #sylvia ::builder::instantiate::InstantiateBuilder> #where_clause {
+                    let msg = #msg_name ::< #(#used_generics),* > ::new( #(#fields_names),* );
+                    let msg = #sylvia ::cw_std::to_json_binary(&msg)?;
+                    Ok( #sylvia ::builder::instantiate::InstantiateBuilder::new(msg, code_id))


Pretty cool that you're exposing a builder API for custom structs decorated with the Sylvia macro!

uint · 2024-10-09T12:00:28Z

sylvia-derive/src/utils.rs

+    &segment.ident
+}
+
+pub fn emit_turbofish(ty: &Type, generics: &[&GenericParam]) -> Type {


This is a good opportunity for a very special message.

jawoznia requested review from hashedone and kulikthebird September 26, 2024 13:27

jawoznia force-pushed the jawoznia/feat/submsg_builder branch from d6c31de to d7e179a Compare September 27, 2024 07:04

feat: Add InstantiateBuilder and SubMsg trait

8140714

jawoznia force-pushed the jawoznia/feat/submsg_builder branch from d7e179a to 8140714 Compare September 27, 2024 08:54

feat: Impl SubMsg trait on CosmosMsg

99a2189

hashedone reviewed Oct 2, 2024

View reviewed changes

kulikthebird reviewed Oct 9, 2024

View reviewed changes

uint approved these changes Oct 9, 2024

View reviewed changes

chore: Review fixes

8747605

jawoznia merged commit 2da70e6 into feat/replies Oct 11, 2024
9 checks passed

jawoznia deleted the jawoznia/feat/submsg_builder branch October 11, 2024 14:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add InstantiateBuilder and SubMsg trait #437

feat: Add InstantiateBuilder and SubMsg trait #437

jawoznia commented Sep 26, 2024

codecov bot commented Sep 26, 2024 •

edited

Loading

hashedone left a comment

hashedone Oct 2, 2024

jawoznia Oct 3, 2024

kulikthebird left a comment

kulikthebird Oct 8, 2024

kulikthebird Oct 8, 2024

jawoznia Oct 9, 2024

kulikthebird Oct 8, 2024

jawoznia Oct 9, 2024

kulikthebird Oct 8, 2024

jawoznia Oct 9, 2024

kulikthebird Oct 9, 2024

jawoznia Oct 9, 2024

uint left a comment

uint Oct 9, 2024

jawoznia Oct 9, 2024

uint Oct 9, 2024

uint Oct 9, 2024

feat: Add InstantiateBuilder and SubMsg trait #437

feat: Add InstantiateBuilder and SubMsg trait #437

Conversation

jawoznia commented Sep 26, 2024

codecov bot commented Sep 26, 2024 • edited Loading

Codecov Report

hashedone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kulikthebird left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 26, 2024 •

edited

Loading