[cosmos] Implement set fee governance instruction #423

jayantk · 2022-12-09T16:36:33Z

Add the set fee governance instruction to the cosmos contract. I thought this would be a good warmup but the change sprawled a bit because I needed to add governance instructions, serialization, etc. I'll leave some comments inline on anything nonobvious.

jayantk · 2022-12-09T22:21:49Z

cosmwasm/contracts/pyth/src/contract.rs

            emitter:            msg.pyth_emitter,
            pyth_emitter_chain: msg.pyth_emitter_chain,
        }]),
+        chain_id:                   msg.chain_id,


added governance fields to ConfigInfo and the initialization message.

Note that fee_denom below seems to be needed to determine which coin the fees are paid in. I don't totally understand what this does, but I'm copying from wormhole here https://github.com/wormhole-foundation/wormhole/blob/main/cosmwasm/contracts/wormhole/src/contract.rs#L74

We should think a little bit about it and we should ask WH people how they handled it. Basically in cosmos there might be more than 1 native coin, as in terra there were both LUNA and UST (iirc: uluna and ust).

So the question is that do we want to have only one accepted coin per each chain?

Due to the nature of cosmwasm I think it might be better if we have multiple Coins and define a separate governance instruction for CW contracts that can set the fee to multiple coins.

Note: I don't know how many chains have multiple native coins. It is worth looking.

wormhole only supports one coin (uluna). We can support multiple, but let me add that in a separate pr. (I think we'll need more governance instructions if we go that route.)

jayantk · 2022-12-09T22:23:18Z

cosmwasm/contracts/pyth/src/contract.rs

@@ -397,7 +513,7 @@ mod test {
        vaa.emitter_address = vec![1u8];
        vaa.emitter_chain = 3;

-        assert_eq!(verify_vaa_sender(&config_info, &vaa), Ok(()));
+        assert_eq!(verify_vaa_from_data_source(&config_info, &vaa), Ok(()));


(this is a rename for clarity, since there are now two different VAA senders)

ali-behjati · 2022-12-13T12:18:27Z

cosmwasm/contracts/pyth/src/msg.rs

-    pub pyth_emitter:       Binary,
-    pub pyth_emitter_chain: u16,
+    pub wormhole_contract:          HumanAddr,
+    pub pyth_emitter:               Binary,


Not related to this PR, but can we change the Inst..Msg to get a list of emitter addresses and chains instead of 1?

ali-behjati · 2022-12-13T12:22:05Z

cosmwasm/contracts/pyth/src/msg.rs

 }

 #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq, JsonSchema)]
 #[serde(rename_all = "snake_case")]
 pub enum ExecuteMsg {
-    SubmitVaa { data: Binary },
+    UpdatePriceFeeds { data: Binary },


Not related to this PR, but would be nice if we add UpdatePriceFeedsIfNecessary in another PR too.

ali-behjati · 2022-12-13T12:24:54Z

cosmwasm/contracts/pyth/src/state.rs

+    pub valid_time_period:          Duration,
+
+    // The fee to pay, denominated in fee_denom (typically, the chain's native token)
+    pub fee:       u128,


Why not storing Coin here? Coin has both fee and denom and also it's native to the cosmwasm api has_coins or transferring all get Coin type.

Also, in general, instead of u128 it's better to use cosmwasm Uint128 which handles json serde properly.

I've been copying the wormhole code for all of this stuff. Let me circle back on this (and the supporting multiple coin types) after implementing the actual payment mechanics, which will help me understand how this works.

ali-behjati

This is great! Sorry that the work is more than what you expected. I left couple of comments.

Also I investigated the tilt problem and that is because of uint128. Apparently in cosmwasm you cannot use it at all in Ser/De (more info here). So you need to use Uint128 here to fix the initialization issue. I tested it and it worked. Other values should be int and only fee should become a string.

ali-behjati · 2022-12-13T12:30:15Z

cosmwasm/contracts/pyth/src/state.rs

+    pub valid_time_period:          Duration,
+
+    // The fee to pay, denominated in fee_denom (typically, the chain's native token)
+    pub fee:       u128,


Also, in general, instead of u128 it's better to use cosmwasm Uint128 which handles json serde properly.

ali-behjati · 2022-12-13T12:32:28Z

cosmwasm/contracts/pyth/src/contract.rs

            emitter:            msg.pyth_emitter,
            pyth_emitter_chain: msg.pyth_emitter_chain,
        }]),
+        chain_id:                   msg.chain_id,


Due to the nature of cosmwasm I think it might be better if we have multiple Coins and define a separate governance instruction for CW contracts that can set the fee to multiple coins.

Note: I don't know how many chains have multiple native coins. It is worth looking.

ali-behjati · 2022-12-13T12:34:09Z

cosmwasm/contracts/pyth/src/contract.rs

-        data_sources:      HashSet::from([PythDataSource {
+        owner:                      info.sender,
+        wormhole_contract:          deps.api.addr_validate(msg.wormhole_contract.as_ref())?,
+        data_sources:               HashSet::from([PythDataSource {
            emitter:            msg.pyth_emitter,
            pyth_emitter_chain: msg.pyth_emitter_chain,
        }]),


In other contracts, we use chain_id from Wormhole, isn't it possible here?

After seeing the tests I realized why you are doing it.

In cosmwasm 1.0.0, there is a way to mock contract queries (it wasn't there in the past for 0.16 in Classic Terra). Please look at this:
https://docs.rs/cosmwasm-std/1.0.0/cosmwasm_std/testing/struct.MockQuerier.html

ali-behjati · 2022-12-13T12:35:19Z

cosmwasm/contracts/pyth/src/state.rs

+
+    // The fee to pay, denominated in fee_denom (typically, the chain's native token)
+    pub fee:       u128,
+    pub fee_denom: String,
 }

 #[derive(Serialize, Deserialize, Clone, Debug, Default, PartialEq, Eq, JsonSchema)]


Not related to this PR, in other networks we decided to only store the price feed and ignore other fields, as storage is expensive.

ali-behjati · 2022-12-13T12:37:31Z

cosmwasm/README.md

@@ -1,6 +1,6 @@
-# Pyth CosmWasm
+l# Pyth CosmWasm


This l is added by mistake here.

ali-behjati · 2022-12-13T12:40:25Z

cosmwasm/contracts/pyth/src/governance.rs

+            return Err(format!("Invalid governance module {module_num}",).into());
+        }
+
+        // TODO: check endianness


wdym here? It seems you are checking it. And yeah we ser/de everything big endian (because EVM is a big indian VM).

the EVM thing is exactly what i was wondering. thanks.

ali-behjati · 2022-12-13T12:41:10Z

cosmwasm/contracts/pyth/src/governance.rs

+        let action_type: u8 = bytes.read_u8()?;
+        let target_chain_id: u16 = bytes.read_u16::<BigEndian>()?;
+
+        let action: Result<GovernanceAction, String> = match action_type {


Nitpick: maybe have a separate enum for this?

ali-behjati · 2022-12-13T12:42:48Z

cosmwasm/contracts/pyth/src/governance.rs

+        Ok(buf)
+    }
+}
+


I think the lines below are unncessary.

ali-behjati · 2022-12-13T13:03:20Z

cosmwasm/contracts/pyth/src/contract.rs

+            updated_config.fee = (val as u128)
+                .checked_mul(
+                    10_u128
+                        .checked_pow(expo as u32)


nitpick: Can we do a checked conversion here instead of expo as u32? as will truncate the value.

ali-behjati

Awesome!

Jayant Krishnamurthy added 7 commits December 6, 2022 09:38

cosmos stuff

666d172

it builds

c059563

fix existing tests

2366980

adding tests

229f5be

adding tests

e5f52ee

tidy

97e8339

cleanup

3e2c268

jayantk changed the title ~~[wip] Implement set fee in cosmos contract~~ Implement set fee in cosmos contract Dec 9, 2022

jayantk commented Dec 9, 2022

View reviewed changes

Jayant Krishnamurthy added 6 commits December 9, 2022 14:23

fix

365e65c

update deploy script

ec2895a

doh

1b56669

gr

f085e36

mabye

5217d5e

mabye

1d0c62f

ali-behjati reviewed Dec 13, 2022

View reviewed changes

ali-behjati changed the title ~~Implement set fee in cosmos contract~~ Implement set fee governance instruction in cosmos contract Dec 13, 2022

ali-behjati changed the title ~~Implement set fee governance instruction in cosmos contract~~ [cosmos] Implement set fee governance instruction Dec 13, 2022

pr comments

1ec7ddd

ali-behjati approved these changes Dec 13, 2022

View reviewed changes

jayantk merged commit 024d73a into main Dec 13, 2022

jayantk deleted the cosmos branch December 13, 2022 16:46

[cosmos] Implement set fee governance instruction #423

[cosmos] Implement set fee governance instruction #423

Uh oh!

Conversation

jayantk commented Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ali-behjati Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ali-behjati Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ali-behjati left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ali-behjati Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ali-behjati left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jayantk commented Dec 9, 2022 •

edited

Loading

ali-behjati Dec 13, 2022 •

edited

Loading

ali-behjati Dec 13, 2022 •

edited

Loading

ali-behjati left a comment •

edited

Loading

ali-behjati Dec 13, 2022 •

edited

Loading