tgrade-valset: Interface for distribution rewards by external contract #198

hashedone · 2021-10-01T12:49:29Z

Closes #138

Instantiate rewards distribution contract on init
Emit event with information about distribution contract on contract instantiation
Allow to query for distribution contract
Distribute rewards by external contract
Tests

…ct on contract instantiation

… is also queryable as config part)

…ract

ethanfrey

In general, this looks quite good and thorough.

The logic for handling the response (MsgInstantiateContract) would be good to pull out as a general utility helper and then see if we use protobuf, prost, or just hard-code a decoding (as there are 2 simple types and we may need that to avoid code bloat). (Created CosmWasm/cw-plus#480 and put it in the backlog)

Other than that, I just think there are a few events and a message that could be optional (only emitted when non-empty). Please ping when you fix those so I can approve and merge

ethanfrey · 2021-10-08T10:41:32Z

contracts/tgrade-valset/proto/instantiate_reply.proto

+syntax = "proto3";
+
+// MsgInstantiateContractResponse defines the Msg/InstantiateContract response type.
+message MsgInstantiateContractResponse {


Nice to pull this out and add the autogen.
Honestly, I find prost easier to work with than the protobuf package.

We could merge like this, but it would be good to pull this into a common helpers package. Eventually optimising for minimum code-size, dependency bloat. I think I have this in another contract or two somewhere. And others ask for it. Something that could be added to cw-plus (as cw-proto package?).

I guess it could be. It is just making maintenance easier (no risk of committing not regenerated message)

those proto types are a stable part of the 1.0 api. they will not change.

ethanfrey · 2021-10-08T10:42:16Z

contracts/tgrade-valset/README.md

+
+```json
+{
+  "_contract_addr": "valset_addr"


missing comma

contracts/tgrade-valset/examples/schema.rs

contracts/tgrade-valset/src/msg.rs

contracts/tgrade-valset/src/contract.rs

ethanfrey · 2021-10-08T10:46:07Z

contracts/tgrade-valset/src/contract.rs

-        .map(|vi| ValidatorUpdate {
-            pubkey: vi.validator_pubkey.clone(),
-            power: vi.power,
+        .map(|vi| {


I like this with unzip to do 2 at once.

ethanfrey · 2021-10-08T10:48:11Z

contracts/tgrade-valset/src/contract.rs

+
+pub fn rewards_instantiate_reply(deps: DepsMut, msg: Reply) -> Result<Response, ContractError> {
+    let id = msg.id;
+    let res: MsgInstantiateContractResponse = Message::parse_from_bytes(


This would be a good logic to pull out into a helper. In addition to the proto logic.

The only time we need to use this struct is handling SubMsg where we want to get info from another contract.
(Pulling into cw-plus can be a separate PR and shouldn't block this one)

ethanfrey · 2021-10-08T10:50:04Z

contracts/tgrade-valset/src/contract.rs


        // diff between empty and vals must be removals
-        let diff = calculate_diff(empty, vals.clone());
-        assert_eq!(diff.diffs.len(), 2);


These updates look fine and fair to remove this length check

Actual reason I did it is that they are only making failure messages worse. You got info test failed, but you don't have diff output, so it may be harder to reason about what happened (you would probably add prints and rerun test...). It would fail anyway line below ;)

ethanfrey · 2021-10-08T10:52:15Z

contracts/tgrade-valset/src/test_helpers.rs

+    pub fn withdraw_validation_reward(&mut self, executor: &str) -> AnyResult<AppResponse> {
+        self.app.execute_contract(
+            Addr::unchecked(executor),
+            self.rewards_contract.clone(),


nice one, letting the tests interact with the rewards contract, just like the main contract.

I was surprised how clean those tests were, but this is very nice abstraction for the suite.

ethanfrey · 2021-10-08T10:53:30Z

contracts/tgrade-valset/src/test_helpers.rs

+        let denom = self.token.clone();
+        let admin = Addr::unchecked(&self.admin);
+        let recipient = self.valset.to_string();
+        self.app.init_modules(move |router, api, storage| {


Very good use of the API.

Question: does it make sense to provide some easier approach for this?
I thought of (maybe did???) add a SudoMsg::Bank to allow minting in some version of multi-test. Can you see if you can use that here? We should make this normal use case easy

Yea, I spend like couple hours to find this function (its name isn't helping here). I would actually just make the .custom_message on app which sends sudo message directly, so it would just look like:

self.app.custom_message(TgradeMsg::MintTokens { denom, amount: amount.into(), recipient, }).unwrap();

ethanfrey

one optimization missing (don't send update messages on no change). Otherwise, good to merge.

maurolacy · 2021-10-08T14:45:32Z

contracts/tgrade-valset/src/contract.rs

        })
-        .collect();
+        .unzip();


maurolacy · 2021-10-08T15:04:22Z

contracts/tgrade-valset/src/contract.rs

+                err: "Missing reply data".to_owned(),
+            })?
+            .as_slice(),
+    )


Very nice. Only question is, do we really need to use protobuf here, or this could be handled with serde and Deserialize / from_binary?

This is protobuf coming from wasmd.
And yes, you do need to parse it.
We never need to create it (it happens when we dispatch a message and check data on reply).

We should document this better here CosmWasm/wasmd#623 and then add helpers here CosmWasm/cw-plus#480

(No rush on these... they are lower in the backlog but good to flag this as an issue)

hashedone force-pushed the 138-distributing-rewards-using-engagement branch from 1d9c411 to 84c2f62 Compare October 1, 2021 12:58

hashedone marked this pull request as draft October 1, 2021 13:04

hashedone force-pushed the 138-distributing-rewards-using-engagement branch 3 times, most recently from 52de51d to d0a2ef0 Compare October 5, 2021 10:51

This comment has been minimized.

Sign in to view

hashedone force-pushed the 138-distributing-rewards-using-engagement branch 2 times, most recently from 7c45ebb to 4e8ef65 Compare October 6, 2021 14:45

hashedone added 7 commits October 7, 2021 10:54

tgrade-valset: Interface for distribution rewards by external contract

ddcf1c9

tgrade-valset: Instantiating rewards distribution contract on init

0c6a81e

tgrade-valset: Emitting information about rewards distribution contra…

828fa62

…ct on contract instantiation

tgrade-valset: Instantiated rewards contract stored in config (and it…

c752e13

… is also queryable as config part)

tgrade-valset: Actual rewards dsitribution done by external contract

6a3db89

tgrade-valset: distriibution over rewards contract tests

5464dd1

tgrade-valset: Tests on updating members in rewards distribution cont…

1f73104

…ract

hashedone force-pushed the 138-distributing-rewards-using-engagement branch from 2b5446b to 1f73104 Compare October 7, 2021 08:55

hashedone requested review from ethanfrey, maurolacy and ueco-jb October 7, 2021 08:55

hashedone marked this pull request as ready for review October 7, 2021 08:55

Reordering lint CI steps

eaf5a8c

ethanfrey reviewed Oct 8, 2021

View reviewed changes

Post review corrections

22c58e4

ethanfrey approved these changes Oct 8, 2021

View reviewed changes

ethanfrey merged commit e13303d into main Oct 8, 2021

ethanfrey deleted the 138-distributing-rewards-using-engagement branch October 8, 2021 13:06

maurolacy reviewed Oct 8, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tgrade-valset: Interface for distribution rewards by external contract #198

tgrade-valset: Interface for distribution rewards by external contract #198

hashedone commented Oct 1, 2021 •

edited

Loading

This comment has been minimized.

ethanfrey left a comment

ethanfrey Oct 8, 2021

hashedone Oct 8, 2021

ethanfrey Oct 8, 2021

ethanfrey Oct 8, 2021

ethanfrey Oct 8, 2021

ethanfrey Oct 8, 2021

ethanfrey Oct 8, 2021

hashedone Oct 8, 2021

ethanfrey Oct 8, 2021

ethanfrey Oct 8, 2021

hashedone Oct 8, 2021

ethanfrey left a comment

maurolacy Oct 8, 2021

maurolacy Oct 8, 2021 •

edited

Loading

ethanfrey Oct 8, 2021

tgrade-valset: Interface for distribution rewards by external contract #198

tgrade-valset: Interface for distribution rewards by external contract #198

Conversation

hashedone commented Oct 1, 2021 • edited Loading

This comment has been minimized.

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

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

maurolacy Oct 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashedone commented Oct 1, 2021 •

edited

Loading

maurolacy Oct 8, 2021 •

edited

Loading