Implementing all messages handling in mutlitest App #398

hashedone · 2021-09-01T11:43:55Z

Closes #388

maurolacy

Looks good.

Handling this into two separate MRs (builder pattern for App, and custom messages handling) would have made reviewing easy for me.

maurolacy · 2021-09-03T07:52:08Z

packages/multi-test/src/app.rs

+/// Also `ExecC` is the custom message which is handled by custom message handler.
+///
+/// `QueryC` is custom query message handled by custom message handler.
+pub struct App<ExecC = Empty, QueryC = Empty>


Just a detail, but why not naming these E, Q?

I am basically enemy of single letter names, they are semanticless. Sometimes in very obvious cases T makes its jobs, but besides I find it wasting more time reading, that saving writing.

maurolacy · 2021-09-03T07:53:48Z

packages/multi-test/src/app.rs

-        let bank = BankKeeper::new();
-
-        App::new(api, env.block, bank, MockStorage::new())
+        AppBuilder::new().build()


maurolacy · 2021-09-03T07:57:33Z

packages/multi-test/src/test_helpers/contracts/echo.rs

+
+#[derive(Debug, Clone, Serialize, Deserialize, Derivative)]
+#[derivative(Default(bound = "", new = "true"))]
+pub struct Message<ExecC>


Suggested change

pub struct Message<ExecC>

pub struct Message<ExecC = Empty>

? (not sure that's possible / useful).

It is possible for sure and I think it is even used. Empty message is still a message, good enough for testing purposes.

packages/multi-test/src/wasm.rs

hashedone · 2021-09-03T09:36:54Z

Yea, it went bigger than I expected :/ I wanted to start with all other messages (stargate and so), but I decided that I will wait for this to be reviewed as it would be too much spam.

ethanfrey

In general, this looks good. I like the builder pattern and thanks for adding some Custom support.

I am rather strongly against using mockall here, as we will not need that in any real usage of multitest (a proper mock object should be provided by one chain). I support a minimal implementation of a custom handler (without mockall) that is just pub(crate) and used for the internal tests.

Each blockchain can provide a proper implementation along with the bindings where they define CosmosMsg and QueryRequest extensions.

ethanfrey · 2021-09-06T15:18:55Z

packages/multi-test/src/custom_handler.rs

+
+/// Simplified version of `CustomHandler` having only arguments which are not app internals - they
+/// are just discarded. Usefull for simpler mocking.
+#[automock(type QueryResult = Binary;)]


I would feel happier removing this and just implementing the 2 needed methods, then mockall can be a dev-dependency only.

In reality, we will likely have much larger blockchain specific logic in the custom handlers and just applying mocks here doesn't make too much sense usually.

Basically this is the idea - specific blockchain impl just adds its own implementation. Auto-generated mock is just to make internal testing easier, but I will get rid of this in this place.

ethanfrey · 2021-09-06T15:22:46Z

packages/multi-test/src/app.rs

+        self
+    }
+
+    pub fn build(self) -> App<ExecC, QueryC> {


Nice way to provide defaults that can be overridden

ethanfrey · 2021-09-06T15:24:11Z

packages/multi-test/src/app.rs

+    /// Overwrites default wasm executor. Panic if already set.
+    #[track_caller]
+    pub fn with_wasm(mut self, wasm: impl Wasm<ExecC, QueryC> + 'static) -> Self {
+        assert!(self.wasm.is_none(), "Wasm executor already overwritten");


We don't allow overriding it twice? I guess no need, but seems odd to enforce.

If you overwrite something twice in your builder then you made some mistake. Better to catch early.

ethanfrey · 2021-09-06T15:25:21Z

packages/multi-test/src/app.rs

-    pub wasm: Box<dyn Wasm<C>>,
-    pub bank: Box<dyn Bank>,
+pub struct Router<ExecC, QueryC> {
+    pub(crate) wasm: Box<dyn Wasm<ExecC, QueryC>>,


I would actually leave these public. But that can be a different PR when there is a need.

Why? Any reason why user of this crate should in any point get access to internals of structure?

Not now, I have an idea, but the need will show up in a future PR, and we can discuss there.
Out of scope here. For now pub(crate) is good.

ethanfrey · 2021-09-06T15:29:08Z

packages/multi-test/src/custom_handler.rs

+    fn query(&self, block: &BlockInfo, msg: QueryC) -> AnyResult<Binary>;
+}
+
+impl<ExecC, QueryC, T: SimpleCustomHandler<ExecC, QueryC>> CustomHandler<ExecC, QueryC> for T {


I would just have this implementation return error (so you can test it works). For any real usage, we will have time to build a module to define the semantics of this. We will want to mock out the real blockchain behaviour, so I would tend against one-off mocks.

Or maybe a FailingCustomHandler, which returns a pre-defined error (set in new?)
And SuccessCustomHandler, which returns a fixed response (set in new?)

One or both just to help in your testing in other packages, but those don't even have to be public outside of this crate.

ethanfrey · 2021-09-06T15:33:37Z

packages/multi-test/src/wasm.rs

    /// code is in-memory lookup that stands in for wasm code
    /// this can only be edited on the WasmRouter, and just read in caches
-    codes: HashMap<usize, Box<dyn Contract<C>>>,
+    codes: HashMap<usize, Box<dyn Contract<ExecC>>>,
+    /// Just marker to make type elision fork when using it as `Wasm` trait


Actually, the contract should have QueryC in it's type.

Update: I looked at this code (also in cosmwasm-std) and it seems the query types are not enforced in DepsMut (which is where they are used). This is proper here, but maybe we can look at making cosmwasm-std more type safe

(TODO: @ethanfrey make an issue on cosmwasm-std)

ethanfrey

Looks better.
I would still hide some more types (make them pub(crate)) to ensure/signal they are only for internal testing of multi-test

ethanfrey · 2021-09-07T12:20:01Z

packages/multi-test/src/custom_handler.rs

+/// is implemented, custom messages should not be send.
+pub struct PanickingCustomHandler;
+
+impl<ExecC, QueryC> SimpleCustomHandler<ExecC, QueryC> for PanickingCustomHandler


Why even have the SimpleCustomHandler trait? Why not just implement CustomHandler directly?

Also, I think CustomHandler is the only type in this file that should be pub (rather than pub(crate)). These other types are only meaningful for us to test multi-test itself (not for blockchains that actually use custom types)

Yea, probably in contract we should not use in contracts tests. SimpleCustomHandler is here to make implementations kind of simpler as I assume that app is somehow accessible on the side, but it may be an overkill.

ethanfrey · 2021-09-07T12:20:18Z

packages/multi-test/src/app.rs

@@ -1594,6 +1582,13 @@ mod test {
                &[],
            )
            .unwrap();
+
+            assert_eq!(


This looks nice.

Better multitest::App creation interface

c0e7522

hashedone marked this pull request as draft September 1, 2021 11:44

hashedone requested review from maurolacy and ueco-jb September 1, 2021 11:44

hashedone added 2 commits September 1, 2021 15:43

Mocking custom messages handler in multitestg

f4ef0b1

Test for handling custom execution messages

c1ee67b

hashedone force-pushed the mt-msg-handling branch from f96a7ba to c1ee67b Compare September 1, 2021 16:15

hashedone marked this pull request as ready for review September 1, 2021 16:15

maurolacy reviewed Sep 3, 2021

View reviewed changes

ethanfrey reviewed Sep 6, 2021

View reviewed changes

Getting rid of mockall in multitest

1f82b41

ethanfrey approved these changes Sep 7, 2021

View reviewed changes

Getting rid of SimpleCustomHandler

a3e574b

hashedone merged commit a918a0f into main Sep 7, 2021

hashedone deleted the mt-msg-handling branch September 7, 2021 16:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing all messages handling in mutlitest App #398

Implementing all messages handling in mutlitest App #398

hashedone commented Sep 1, 2021

maurolacy left a comment •

edited

Loading

maurolacy Sep 3, 2021

hashedone Sep 3, 2021

maurolacy Sep 3, 2021

maurolacy Sep 3, 2021

hashedone Sep 3, 2021

hashedone commented Sep 3, 2021

ethanfrey left a comment

ethanfrey Sep 6, 2021

ethanfrey Sep 6, 2021

hashedone Sep 6, 2021

ethanfrey Sep 6, 2021

ethanfrey Sep 6, 2021

hashedone Sep 6, 2021

ethanfrey Sep 6, 2021

hashedone Sep 6, 2021

ethanfrey Sep 6, 2021 •

edited

Loading

ethanfrey Sep 6, 2021

ethanfrey Sep 6, 2021

ethanfrey Sep 6, 2021

ethanfrey left a comment

ethanfrey Sep 7, 2021

hashedone Sep 7, 2021

ethanfrey Sep 7, 2021

Implementing all messages handling in mutlitest App #398

Implementing all messages handling in mutlitest App #398

Conversation

hashedone commented Sep 1, 2021

maurolacy left a comment • edited Loading

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 commented Sep 3, 2021

ethanfrey 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

ethanfrey Sep 6, 2021 • edited Loading

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

ethanfrey 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

maurolacy left a comment •

edited

Loading

ethanfrey Sep 6, 2021 •

edited

Loading