Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: re-add support for gov v1beta1 messages #725

Merged
merged 5 commits into from
Apr 17, 2024

Conversation

dadamu
Copy link
Contributor

@dadamu dadamu commented Mar 21, 2024

Description

Closes: #XXXX
Currently, gov module is updated to gov v1, however, gov v1beta1 is still alive to chains, Cosmos-SDK doesn't completely remove v1beta1 support. As a result, we should re-add gov v1beta1 support to make sure our proposals data are also updated when gov v1beta1 messages are performed.

Say, some of users on Cheqd is using gov.v1beta1.MsgVote to vote on the proposal.
https://bigdipper.live/cheqd/transactions/19E4F068FBFBC46DA9BEF3E0F7CA908317D5582CAB2A17683138A5248ED34E3C


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch
  • provided a link to the relevant issue or specification
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

Comment on lines +108 to +111
err = proposal.UnpackInterfaces(m.cdc)
if err != nil {
return fmt.Errorf("error while unpacking proposal interfaces: %s", err)
}
Copy link
Contributor Author

@dadamu dadamu Mar 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using codec to unpack Any type messages to the underlying structure, then we can get proposal message by proposal.Messages without error.

// handleMsgSubmitProposal allows to properly handle a MsgSubmitProposal
func (m *Module) handleMsgSubmitProposal(tx *juno.Tx, index int, msg *govtypesv1.MsgSubmitProposal) error {
// handleSubmitProposalEvent allows to properly handle a handleSubmitProposalEvent
func (m *Module) handleSubmitProposalEvent(tx *juno.Tx, proposer string, events sdk.StringEvents) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

V1beta1 and V1 messages are sharing the same event, so we can parse event rather than push message directly so that we don't support many version messages later.

https://docs.cosmos.network/main/build/modules/gov#events

Comment on lines +41 to +42
// 1. "{\"option\":1,\"weight\":\"1.000000000000000000\"}"
// 2. "option:VOTE_OPTION_NO weight:\"1.000000000000000000\""
Copy link
Contributor Author

@dadamu dadamu Mar 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VoteOption inside event is represented in these two type format, v1 and v1beta1 currently.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still trying to understand cosmos SDK.

We are not able to use provided methods like:

func (m *Vote) GetOptions() []*WeightedVoteOption {
	if m != nil {
		return m.Options
	}
	return nil
}


case *govtypesv1.MsgVote:
return m.handleMsgVote(tx, cosmosMsg)
return m.handleVoteEvent(tx, cosmosMsg.Voter, tx.Logs[index].Events)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Voter event is supported by 0.47.5, we currently better to put Voter from message to make it backward compatible.
https://github.com/cosmos/cosmos-sdk/blob/main/CHANGELOG.md#v0475---2023-09-01

@@ -32,37 +30,35 @@ func (m *Module) HandleMsg(index int, msg sdk.Msg, tx *juno.Tx) error {

switch cosmosMsg := msg.(type) {
case *govtypesv1.MsgSubmitProposal:
return m.handleMsgSubmitProposal(tx, index, cosmosMsg)
return m.handleSubmitProposalEvent(tx, cosmosMsg.Proposer, tx.Logs[index].Events)
Copy link
Contributor Author

@dadamu dadamu Mar 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need put proposer from message since Cosmos-SDK does not support proposer attribute inside submit_proposal event, it maybe better we post a request to Cosmos-SDK team or just remove this field in the feature. From my side, I would remove it as I would see depositors more than proposer, in addition, proposer must be one of depositors.

@dadamu dadamu marked this pull request as draft March 21, 2024 09:14
}

// handleMsgVoteWeighted allows to properly handle a MsgVoteWeighted
func (m *Module) handleMsgVoteWeighted(tx *juno.Tx, msg *govtypesv1.MsgVoteWeighted) error {
Copy link
Contributor Author

@dadamu dadamu Mar 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shares the same vote event as MsgVote, so it can be removed safely.

@dadamu dadamu marked this pull request as ready for review March 21, 2024 11:01
Copy link

@0x7u 0x7u left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions :)

Comment on lines 134 to 135
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about

		switch msg.(type) {
		case *govtypesv1.MsgSubmitProposal, *govtypesv1beta1.MsgSubmitProposal:
			err = govModule.HandleMsg(index, msg, tx)
			if err != nil {
				return fmt.Errorf("error while handling MsgSubmitProposal: %s", err)
			}

		}

Copy link
Contributor Author

@dadamu dadamu Mar 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, it is cleaner, thanks! 3936cc6

Comment on lines +41 to +42
// 1. "{\"option\":1,\"weight\":\"1.000000000000000000\"}"
// 2. "option:VOTE_OPTION_NO weight:\"1.000000000000000000\""
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still trying to understand cosmos SDK.

We are not able to use provided methods like:

func (m *Vote) GetOptions() []*WeightedVoteOption {
	if m != nil {
		return m.Options
	}
	return nil
}

@dadamu
Copy link
Contributor Author

dadamu commented Mar 26, 2024

@0x7u

We are not able to use provided methods like:

Unluckily, the thing I tried is to parse the string from event, and it shows two cases in Cosmos-SDK now.
The option value string is from:
https://github.com/cosmos/cosmos-sdk/blob/release/v0.47.x/x/gov/types/v1beta1/vote.go#L66
https://github.com/cosmos/cosmos-sdk/blob/release/v0.47.x/x/gov/types/v1/gov.pb.go#L133

There is no function supporting to parse string into option and also WeightOptionsFromString couldn't parse these option string properly since it is used for cmd string.

Therefore we can only parse them by ourselves. If we handle message instance directly, then we need to have both functions, v1 and v1beta1 for each gov messages.

@0x7u
Copy link

0x7u commented Mar 27, 2024

@0x7u

We are not able to use provided methods like:

Unluckily, the thing I tried is to parse the string from event, and it shows two cases in Cosmos-SDK now. The option value string is from: https://github.com/cosmos/cosmos-sdk/blob/release/v0.47.x/x/gov/types/v1beta1/vote.go#L66 https://github.com/cosmos/cosmos-sdk/blob/release/v0.47.x/x/gov/types/v1/gov.pb.go#L133

There is no function supporting to parse string into option and also WeightOptionsFromString couldn't parse these option string properly since it is used for cmd string.

Therefore we can only parse them by ourselves. If we handle message instance directly, then we need to have both functions, v1 and v1beta1 for each gov messages.

Very interesting.

@dadamu dadamu merged commit cf05236 into cosmos/v0.47.x Apr 17, 2024
5 checks passed
@dadamu dadamu deleted the paul/re-add-gov-v1beta1-support branch April 17, 2024 06:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants