Generate custom responses #156

jawoznia · 2023-06-07T14:55:00Z

Closes #155

codecov · 2023-06-07T15:08:29Z

Codecov Report

Merging #156 (0e1f3eb) into custom_msg_support (196cad0) will decrease coverage by 0.15%.
The diff coverage is 64.86%.

❗ Current head 0e1f3eb differs from pull request most recent head 5379712. Consider uploading reports for the commit 5379712 to get more accurate results

@@                  Coverage Diff                   @@
##           custom_msg_support     #156      +/-   ##
======================================================
- Coverage               81.79%   81.64%   -0.15%     
======================================================
  Files                      36       36              
  Lines                    1774     1776       +2     
======================================================
- Hits                     1451     1450       -1     
- Misses                    323      326       +3

Impacted Files	Coverage Δ
sylvia-derive/src/message.rs	`74.53% <43.75%> (-1.72%)`	⬇️
sylvia/tests/custom_msg.rs	`63.63% <60.00%> (+38.63%)`	⬆️
sylvia-derive/src/multitest.rs	`76.56% <70.00%> (+1.82%)`	⬆️
sylvia-derive/src/input.rs	`78.94% <70.58%> (-5.99%)`	⬇️
sylvia-derive/src/parser.rs	`81.57% <70.58%> (+0.77%)`	⬆️
sylvia-derive/src/lib.rs	`73.91% <100.00%> (-1.09%)`	⬇️

... and 5 files with indirect coverage changes

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

hashedone

That is more or less right, but there are three problems:

Custom messages do not work for traits at all. I am pretty sure you cannot implement any trait for contracts using ExecC. If I am wrong, please at least prove mi wrong by creating a test with a contract using ExecC and the interface implemented on it via trait. This is critical, and we cannot merge it for now. I will come back to that at the end of the comment as it is not trivial.
App which is not BasicApp will not work with those proxies. The solution for that is delivered in the comments, and it is fairly easy, so I expect it to be fixed in this PR.
If a contract has ExecC defined, it is impossible to return a non-customized Response from it, which I find an inconvenience - you previously implemented conversion for Response types just to provide this functionality so we want it to be implemented. I am Ok with creating a separate issue for that and implementing it later, but we want this.

Now we need to understand how custom messages relate to interfaces. We need to recognize cases:

Contract uses Empty ExecC (not defines it at all), and the interface uses Empty ExecC (wherever that means for now). The contract should be able to implement this interface.
Contract uses some custom ExecC, and it wants to implement the trait which is not using ExecC at all (defaults it to Empty). Such a contract should be able to implement his interface, and all Responses returned from interface functions should be converted to contract Response<ExecC> somewhere in the dispatcher.
We want to create a trait using a particular ExecC (most probably designed for a specific chain) in its messages. For now, we want only contracts with the very same ExecC to implement such an interface. In the future, we will introduce the notion of compatibility between ExecC so we will make it possible to implement such trait by any contract which we can convert ExecC properly, but it is for the future only.
We may want to implement the trait which uses the ExecC being sort of "configurable" by chain, so for example have it as an associated type. In such a case there should be a possibility to determine by Sylvia what is the final type we are working with, and it would be delivered via [sv::custom(...)] on the trait impl block.

For now, we need at least 1 or 2 workings and tested, so we fully support custom types on contract (we can support it on traits on separate PR). 1 should be working with now. My intuition is, that if you provide the #[custom(...)] on the trait impl block, and then try to convert the result from functions to the contract response in the contract level impl block (when the trait level dispatcher is called). Possibly we would need to additionally add the #[sv::custom(...)]on the#[interface]if one wants to use a non-emptyExecCinterface (it would be useable either by very concrete type for 3, theSelf::AssociatedCustomTypefor 4, and even generics for some strange cases I don't see) - however we can add this in another PR (so, for now, we would "support" traits usingExecC`, except with no possibility of creating them).

sylvia-derive/src/input.rs

sylvia-derive/src/multitest.rs

sylvia/tests/custom_msg.rs

hashedone

Almost ok, with some minor comments.

My biggest issue is commit history - to detail, it would look bad both in the changelog and in the commit history. Avoid using chore, test and refactor commits. Those are most probably changes to be done either:

as part of the bigger change (eg. test of the feature should go with the feature)
in a completely separated review (eg. if it is a big refactor, then we create the whole refactor pr).

In this case - all of your test commits are to be squashed with parents, and chore commits should come with the next non-chore commit.

We will use those test, refactor and chore when we for example create a single PR filling up some more missing regression in a single PR not adding any new functionality, etc.

I am ok you working on small changes yourself, but then rebase it to keep clean commit history before PR.

hashedone · 2023-06-09T13:06:52Z

sylvia-derive/src/parser.rs

@@ -389,13 +383,21 @@ impl Custom {
            1 => custom.into_iter().next().unwrap(),
            _ => {
                emit_error!(
-                    source.span(),
+                    span,


I should probably spot it in the previous PR, but I did not so we will fix it here.

Now you are giving a short error, passing the span to the whole entity (created from the whole trait!). Misleading.

Also, it doesn't exactly match how similar errors are returned in Rust. E.g., you create a function with a name already existing, Rust error:

error[[E0428]](https://doc.rust-lang.org/stable/error_codes/E0428.html): the name `foo` is defined multiple times --> src/main.rs:2:1 | 1 | fn foo() {} | -------- previous definition of the value `foo` here 2 | fn foo() {} | ^^^^^^^^ `foo` redefined here | = note: `foo` must be defined only once in the value namespace of this module

Do similar:

| #[custom(...)] | ------------ The attribute `custom` is redefined =note: | #[custom(...)] |------------- Previous definition of the attribute `custom` =note: Only one `custom` attribute can exist on a single sylvia entity

You can achieve that with more advanced usage of emit_error. I imagine something like this:

emit_error!( // `attr` is the whole `attr` attribute which is not "first" (obsolete) attr, "The attribute `custom` is redefined"; // `custom` here is the first "proper" `custom` attribute defined note = custom => "Previous definition of the attribute `custom`"; note = "Only one `custom` attribute can exist on a single sylvia entity" )

I know that custom spans are working only on Rust nightly, but it is still beneficial - if I am struggling I will often use nightly for my day-2-day builds because of better error reporting.

One thing to check how those errors look on stable; if there are some artifacts we can always make "spanned" notes optional (with =?) and remove them under some nightly feature flag (but I would leave it enabled-by-default if errors don't look too bad on stable).

Also, we need some way of testing if the errors are reasonably readable - you suggested a solution, but I rejected it because I don't like error message testing too much. However, I was wrong - here it is too critical to maintain them as clearly as possible - please create an issue about using the tool you suggested before; we will try how it works for us.

Additional hint on how to implement this:

let mut customs = attrs .iter() .filter(|attr| ...) .filter_map(|attr| ...); let custom = match customs .next() .unwrap_or_default(); for redefined in customs { emit_error!( redefined, "The attribute `custom` is redefined"; note = custom => "Previous definition of the attribute `custom`"; note = "Only one `custom` attribute can exist on a single sylvia entity"; ); } custom

Benefit: I just got rid of the collection to vec!

I should probably spot it in the previous PR, but I did not so we will fix it here.

Now you are giving a short error, passing the span to the whole entity (created from the whole trait!). Misleading.

Also, it doesn't exactly match how similar errors are returned in Rust. E.g., you create a function with a name already existing, Rust error:

error[[E0428]](https://doc.rust-lang.org/stable/error_codes/E0428.html): the name `foo` is defined multiple times --> src/main.rs:2:1 | 1 | fn foo() {} | -------- previous definition of the value `foo` here 2 | fn foo() {} | ^^^^^^^^ `foo` redefined here | = note: `foo` must be defined only once in the value namespace of this module

Do similar:

| #[custom(...)] | ------------ The attribute `custom` is redefined =note: | #[custom(...)] |------------- Previous definition of the attribute `custom` =note: Only one `custom` attribute can exist on a single sylvia entity

You can achieve that with more advanced usage of emit_error. I imagine something like this:

emit_error!( // `attr` is the whole `attr` attribute which is not "first" (obsolete) attr, "The attribute `custom` is redefined"; // `custom` here is the first "proper" `custom` attribute defined note = custom => "Previous definition of the attribute `custom`"; note = "Only one `custom` attribute can exist on a single sylvia entity" )

I know that custom spans are working only on Rust nightly, but it is still beneficial - if I am struggling I will often use nightly for my day-2-day builds because of better error reporting.

One thing to check how those errors look on stable; if there are some artifacts we can always make "spanned" notes optional (with =?) and remove them under some nightly feature flag (but I would leave it enabled-by-default if errors don't look too bad on stable).

Also, we need some way of testing if the errors are reasonably readable - you suggested a solution, but I rejected it because I don't like error message testing too much. However, I was wrong - here it is too critical to maintain them as clearly as possible - please create an issue about using the tool you suggested before; we will try how it works for us.

Issue to consider trybuild #98

There is a pending PR.
For examples of usage in real projects you can check its use in serde

hashedone · 2023-06-09T13:15:02Z

sylvia-derive/src/multitest.rs

@@ -248,7 +258,7 @@ impl<'a> MultitestHelpers<'a> {
                            #sylvia ::multitest::MigrateProxy::new(&self.contract_addr, msg, &self.app)
                    }
                }
-            } else {
+            } else if msg_ty == &MsgType::Query {


I am not a fan of this if-else chain. I am pretty sure it should be a match or even a function like MsgType::generate_message_variant(...) (not sure about this one, as maybe it is not the best place to put this).

Here you did only minimal changes on this, but let's fix it up in some later refactor (I'll create a task).

Especially since those things look identical. Really. I see the difference, but I think we could simplify it. Not mentioning last else is bullcrap here, I don't see which patterns are not covered; it frustrates me.

hashedone · 2023-06-09T13:29:14Z

.github/workflows/ci.yml

+      # TODO: Remove when custom_msg_support will be merged to main
+      #       Later name feature branches with `feat_` prefix
+      - custom_msg_support
+      - feat_* 


Nice, but I will avoid any _ prefixing in branch names - / feels far more natural (and I am pretty sure it is allowed in branch names). I will go either with feat/* or shorter ll/ (long-living - potentially not only for features but maybe bigger refactors or something`,

hashedone · 2023-06-09T13:31:19Z

.github/workflows/ci.yml

    branches:
      - main
+      # TODO: Remove when custom_msg_support will be merged to main
+      #       Later name feature branches with `feat_` prefix
+      - custom_msg_support
+      - feat_* 


Here I would altogether remove the branches filter. All pull requests should trigger CI flow. We could potentially do a quick CI flow if we have some long-running non-critical CI flows on all PRs, and the full CI only for PRs to main, but I think we don't have this kind of split for now.

sylvia-derive/src/message.rs

… macro

hashedone

LGTM, minor docs wording hints.

hashedone · 2023-06-12T12:30:24Z

.github/workflows/ci.yml

@@ -4,9 +4,11 @@ on:
  push:
    branches:
      - main
+      # TODO: Remove when custom_msg_support will be merged to main
+      #       Later name feature branches with `feat_` prefix


Suggested change

# Later name feature branches with `feat_` prefix

# Later name feature branches with `feat/` prefix

hashedone · 2023-06-12T12:31:19Z

README.md

@@ -842,5 +842,7 @@ features for you. Here is a rough roadmap for the incoming months:

 ## Troubleshooting

+For more descriptive error messages consider using +nightly toolchain.


Suggested change

For more descriptive error messages consider using +nightly toolchain.

For more descriptive error messages, consider using the nightly toolchain (add `+nightly` argument for cargo)

jawoznia requested a review from hashedone June 7, 2023 14:55

jawoznia mentioned this pull request Jun 7, 2023

Response conversion in generated code #149

Closed

hashedone reviewed Jun 7, 2023

View reviewed changes

sylvia-derive/src/input.rs Show resolved Hide resolved

sylvia-derive/src/multitest.rs Outdated Show resolved Hide resolved

sylvia/tests/custom_msg.rs Show resolved Hide resolved

jawoznia changed the base branch from main to custom_msg_support June 8, 2023 15:14

jawoznia force-pushed the generate_custom_responses branch from 2fbd042 to fea6fac Compare June 8, 2023 15:24

hashedone reviewed Jun 9, 2023

View reviewed changes

This was referenced Jun 9, 2023

Refactor some if-else chains to match or functions on matched item #169

Open

Better spanning on generated code #170

Open

jawoznia force-pushed the generate_custom_responses branch from 7081d77 to 0e1f3eb Compare June 9, 2023 16:16

jawoznia added 4 commits June 9, 2023 18:17

feat: Use custom msg type in generated code

9b50c2e

feat: Use more generic MtApp in place of BasicApp

3b3f8af

build: Run CI on push and PR on feature branches

0ca2963

feat: Removed support for module and msg_type attribute for Interface…

a1e902d

… macro

hashedone approved these changes Jun 12, 2023

View reviewed changes

hashedone mentioned this pull request Jun 12, 2023

Custom msg trait support #173

Merged

jawoznia force-pushed the generate_custom_responses branch from 0e1f3eb to 5379712 Compare June 12, 2023 15:09

jawoznia added 2 commits June 12, 2023 17:09

feat: More descriptive error message for sv::custom attribute

274da7b

build: Run CI flow on PR to every branch

5379712

jawoznia merged commit 06f4199 into custom_msg_support Jun 12, 2023
4 checks passed

jawoznia deleted the generate_custom_responses branch June 12, 2023 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate custom responses #156

Generate custom responses #156

jawoznia commented Jun 7, 2023 •

edited by hashedone

codecov bot commented Jun 7, 2023 •

edited

hashedone left a comment

hashedone left a comment

hashedone Jun 9, 2023

hashedone Jun 9, 2023 •

edited

jawoznia Jun 9, 2023

hashedone Jun 9, 2023

hashedone Jun 9, 2023

hashedone Jun 9, 2023

hashedone Jun 9, 2023

hashedone left a comment

hashedone Jun 12, 2023

hashedone Jun 12, 2023

	# Later name feature branches with `feat_` prefix
	# Later name feature branches with `feat/` prefix

		@@ -842,5 +842,7 @@ features for you. Here is a rough roadmap for the incoming months:

		## Troubleshooting

		For more descriptive error messages consider using +nightly toolchain.

	For more descriptive error messages consider using +nightly toolchain.
	For more descriptive error messages, consider using the nightly toolchain (add `+nightly` argument for cargo)

Generate custom responses #156

Generate custom responses #156

Conversation

jawoznia commented Jun 7, 2023 • edited by hashedone

codecov bot commented Jun 7, 2023 • edited

Codecov Report

hashedone left a comment

Choose a reason for hiding this comment

hashedone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashedone Jun 9, 2023 • edited

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

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

jawoznia commented Jun 7, 2023 •

edited by hashedone

codecov bot commented Jun 7, 2023 •

edited

hashedone Jun 9, 2023 •

edited