Upgrade and UpdateClient proposals for Tgrade #191

hashedone · 2022-10-05T13:15:53Z

Addresses #142, #143.

I am not perfectly sure if it is what was expected, but this is what I understood - any leads about what I missed are welcome. Besides adding messages to tgrade gov proposals I added an upgrade plan validation - the name cannot be empty, and the height has to be greater or equal to the current height. A height field in the plan is required, as time-based upgrades are deprecated and should never be sent.

maurolacy

Looks good. Some comments on naming and docs / references.

Just checked tgrade sources and at this point there's no implementation for these governance proposals. So I guess the references below must be to the Cosmos SDK.

maurolacy · 2022-10-10T14:03:00Z

packages/bindings/src/gov.rs

+    // reached and the software will exit.
+    pub name: String,
+    // The height at which the upgrade must be performed.
+    // Only used if Time is not set.


What Time? Remove this comment if not applicable.

fyi: time was deprecated and must not be set

Obviously - I completely removed Time as it is deprecated, but forgot to get rid in doc-comments.

maurolacy · 2022-10-10T14:07:10Z

packages/bindings/src/gov.rs

@@ -86,6 +85,48 @@ pub enum GovProposal {
        /// all code ideas that should be removed from cache to free space
        code_ids: Vec<u64>,
    },
+    // ClientUpdateProposal is a governance proposal. If it passes, the substitute


Doc comments? Also, a url reference to the relevant Cosmos SDK / tgrade sources section would be good.

maurolacy · 2022-10-10T14:07:36Z

packages/bindings/src/gov.rs

+    },
+    // UpgradeProposal is a gov Content type for initiating an IBC breaking
+    // upgrade.
+    Upgrade {


Shouldn't better be called IbcUpgrade or so?

I don't know. @alpe does it? I tried to figure it out but it is not clear to me.

maurolacy · 2022-10-10T14:09:39Z

packages/bindings/src/gov.rs

+    // client's latest consensus state is copied over to the subject client. The proposal
+    // handler may fail if the subject and the substitute do not match in client and
+    // chain parameters (with exception to latest height, frozen height, and chain-id).
+    ClientUpdate {


Isn't this meant to replace IbcClientUpdate, above?

Also, the relevant message in ibc-go 3.1.0 has two extra string fields, title and description.

title and description seems to be in every message and in rust implementation they are just extracted directly to ExecuteGovProposal (https://github.com/confio/poe-contracts/blob/ibc-client-update-proposal-fix/packages/bindings/src/msg.rs#L36-L37). It is not 100% clear to me.

maurolacy · 2022-10-10T14:10:18Z

packages/bindings/src/gov.rs

+
+// Plan specifies information about a planned upgrade and when it should occur.
+#[derive(Serialize, Deserialize, Clone, PartialEq, Eq, PartialOrd, Ord, JsonSchema, Debug)]
+pub struct UpgradePlan {


Better name it IbcUpgradePlan?

I try to keep consistency and it I feel like there is no Ibc prefix even for Ibc stuff here, but I am not sure. I would like to understand the context more and have an understanding of the naming consistency here, I admit I feel a bit lost on that. Personally I would separate all ibc-related things to its own type probably, I don't know.

maurolacy · 2022-10-10T14:12:14Z

contracts/tgrade-validator-voting/src/msg.rs

@@ -101,6 +100,16 @@ pub enum ValidatorProposal {
        /// The contract address to be cleared
        contract: String,
    },
+    ClientUpdate {


Here too, a proper Ibc prefix would be nice.

It is very important to not have this mixed with a chain upgrade.

Suggested change

ClientUpdate {

IBCClientUpdate {

IBC is an acronym and uppercase

@alpe in CamelCase acronyms are done with capitalizing only first letter. I already had issues with tools using it this common way. Same thing as we tend to name types as TcpClient, HttpRequest and so. So if it is to be prefixed, it would be actually IbcClientUpdate.

maurolacy · 2022-10-10T14:12:24Z

contracts/tgrade-validator-voting/src/msg.rs

+        /// The substitute client identifier for the client standing in for the subject client
+        substitute_client_id: String,
+    },
+    Upgrade {


And here too, a proper Ibc prefix to differentiate this update proposal from the chain upgrade one.

Suggested change

Upgrade {

IBCUpgrade {

Thanks for the comments.

As I understand it, this message should go entirely, as it's a duplicate of the RegisterUpgrade message above. As such, it's also not related to IBC.

Then, in the GovProposal enum (bindings/src/gov.rs) , both ClientUpdate and Upgrade should go, and the current IBCClientUpdate message should be properly updated.

maurolacy

On a second look, found this:

https://github.com/confio/tgrade/blob/386fbed230e6d4e6a3ab080a1f9c12848e66192f/x/twasm/contract/incoming_msgs.go#L61-L128

The IBCClientUpdate points to a ClientUpdateProposal in ibc-go v3.1.0.

Looks like that's the one we need to target. Can anyone confirm this? @alpe.

maurolacy · 2022-10-10T15:00:16Z

~~Approved this in haste~~ (already dismissed), but seems some changes are required:

The underlying IBC client update (ibc-go v3.1.0) message has two extra fields.
Not sure 3.1.0 is the right ibc-go version we need to target.
Seems to me the two messages here, Upgrade and ClientUpdate should be removed, and the only needed thing is to update the IbcClientUpdate message with the proper fields, targeting the proper ibc-go version (which we still need to define / agree upon).

Approved in haste

hashedone · 2022-10-10T15:23:04Z

@maurolacy I completely agree it may be the case. I think I would like to have confirmation about it from someone who has an exact understanding of what is needed there as I didn't find proper documentation for that.

alpe

Beside adding the IBC prefix to Upgrade and ClientUpdate it looks good.
Is the duplication in msg and gov.rs needed? It is the same data. 🤷

alpe · 2022-10-12T11:02:18Z

contracts/tgrade-validator-voting/src/msg.rs

@@ -101,6 +100,16 @@ pub enum ValidatorProposal {
        /// The contract address to be cleared
        contract: String,
    },
+    ClientUpdate {


It is very important to not have this mixed with a chain upgrade.

Suggested change

ClientUpdate {

IBCClientUpdate {

IBC is an acronym and uppercase

alpe · 2022-10-12T11:03:59Z

contracts/tgrade-validator-voting/src/msg.rs

+        /// The substitute client identifier for the client standing in for the subject client
+        substitute_client_id: String,
+    },
+    Upgrade {


Suggested change

Upgrade {

IBCUpgrade {

alpe · 2022-10-12T11:10:32Z

packages/bindings/src/gov.rs

+    // reached and the software will exit.
+    pub name: String,
+    // The height at which the upgrade must be performed.
+    // Only used if Time is not set.


fyi: time was deprecated and must not be set

alpe · 2022-10-12T11:12:50Z

contracts/tgrade-validator-voting/src/validate.rs

+                    return Err(ContractError::EmptyUpgradeName {});
+                }
+
+                if height < &env.block.height {


hashedone · 2022-10-31T11:36:02Z

So now there is a question - the old IbcClientUpdate should be completely removed? That is my guess but I am not sure at this point.

maurolacy · 2022-11-01T21:41:13Z

So now there is a question - the old IbcClientUpdate should be completely removed? That is my guess but I am not sure at this point.

Either updated or removed. See this previous comment #191 (comment) for reference.

hashedone requested a review from ethanfrey October 5, 2022 13:15

Upgrade and UpdateClient proposals for Tgrade

0820261

hashedone force-pushed the ibc-client-update-proposal-fix branch from 48336f5 to 0820261 Compare October 5, 2022 13:27

hashedone requested a review from maurolacy October 6, 2022 08:01

daniellarita assigned hashedone Oct 10, 2022

maurolacy previously approved these changes Oct 10, 2022

View reviewed changes

maurolacy reviewed Oct 10, 2022

View reviewed changes

alpe reviewed Oct 12, 2022

View reviewed changes

hashedone added 2 commits October 31, 2022 12:37

Doc comments update

d11dd24

Prefixing IBC related messages with Ibc

403f0bb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade and UpdateClient proposals for Tgrade #191

Upgrade and UpdateClient proposals for Tgrade #191

hashedone commented Oct 5, 2022

maurolacy left a comment •

edited

Loading

maurolacy Oct 10, 2022

alpe Oct 12, 2022

hashedone Oct 31, 2022

maurolacy Oct 10, 2022

maurolacy Oct 10, 2022

hashedone Oct 31, 2022

maurolacy Oct 10, 2022

maurolacy Oct 10, 2022 •

edited

Loading

hashedone Oct 31, 2022

maurolacy Oct 10, 2022 •

edited

Loading

hashedone Oct 31, 2022

maurolacy Oct 10, 2022

alpe Oct 12, 2022

hashedone Oct 31, 2022

maurolacy Oct 10, 2022

alpe Oct 12, 2022

maurolacy Oct 12, 2022 •

edited

Loading

maurolacy left a comment •

edited

Loading

maurolacy commented Oct 10, 2022 •

edited

Loading

hashedone commented Oct 10, 2022

alpe left a comment

alpe Oct 12, 2022

alpe Oct 12, 2022

alpe Oct 12, 2022

alpe Oct 12, 2022

hashedone commented Oct 31, 2022

maurolacy commented Nov 1, 2022

Upgrade and UpdateClient proposals for Tgrade #191

Are you sure you want to change the base?

Upgrade and UpdateClient proposals for Tgrade #191

Conversation

hashedone commented Oct 5, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maurolacy Oct 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maurolacy Oct 10, 2022 • 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

Choose a reason for hiding this comment

maurolacy Oct 12, 2022 • edited Loading

Choose a reason for hiding this comment

maurolacy left a comment • edited Loading

Choose a reason for hiding this comment

maurolacy commented Oct 10, 2022 • edited Loading

hashedone commented Oct 10, 2022

alpe 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

hashedone commented Oct 31, 2022

maurolacy commented Nov 1, 2022

maurolacy left a comment •

edited

Loading

maurolacy Oct 10, 2022 •

edited

Loading

maurolacy Oct 10, 2022 •

edited

Loading

maurolacy Oct 12, 2022 •

edited

Loading

maurolacy left a comment •

edited

Loading

maurolacy commented Oct 10, 2022 •

edited

Loading