feat:added option extension in the protobuf files#3627
feat:added option extension in the protobuf files#3627crodriguezvega merged 8 commits intocosmos:mainfrom
Conversation
|
@crodriguezvega can you please look into the changes i made , if it is correct ,i'll proceed |
|
the idea here is to use the option as defined by the sdk, not define one for ibc. I.e where applicable, bring in the option: and then denote the signer: hope this helps out! |
vishal-kanna
requested review from
AdityaSripal,
DimitrisJim,
charleenfei,
chatton,
colin-axner,
crodriguezvega and
damiannolan
as code owners
vishal-kanna
changed the title
|
hey @vishal-kanna, due to a recent PR which added some additional proto files (see the mentioned PR), we'd need to rebase and add the option to them too. To reduce the amount of rebasing you'd need to do, I'd suggest we wait until all PRs that add new messages are merged and then I'll ping you to add the option to the new messages too. How does that sound? |
|
@DimitrisJim it sounds great.Ping me once all the PRs are merged. |
# Conflicts: # modules/apps/transfer/types/tx.pb.go # proto/ibc/applications/transfer/v1/tx.proto
I think I would prefer to merge this PR as is (I have rebased and added the option extension in the missing messages) and then add the extension in the PRs that are already open for the migration of params. Otherwise, we might postpone merging this PR for too long. |
crodriguezvega
left a comment
crodriguezvega
left a comment
There was a problem hiding this comment.
Thank you, @vishal-kanna. This looks good to me!
| import "gogoproto/gogo.proto"; | ||
| import "ibc/applications/fee/v1/fee.proto"; | ||
| import "ibc/core/channel/v1/channel.proto"; | ||
| import "cosmos/msg/v1/msg.proto"; |
There was a problem hiding this comment.
I remember this is a big nit for Damian, out of order import paths. Though I've opened #3651 in response to this since many of our protos don't respect it.
There was a problem hiding this comment.
lol, I won't lose sleep over it
|
Makes sense to me @crodriguezvega. Note that we might not want to close the original issue as of yet since we still have the new upgrade messages to annotate. |
I would still close the issue and keep in mind that from now on we can add the extension to any new message we add. And for channel upgradability, we can open a separate PR to add it to the messages added in that feature branch. :) EDIT: I opened a PR for 04-channel-upgrades. |
# Conflicts: # modules/core/02-client/types/tx.pb.go # proto/ibc/core/client/v1/tx.proto
# Conflicts: # modules/core/02-client/types/tx.pb.go
5 participants
Description
closes: #3624
Commit Message / Changelog Entry
see the guidelines for commit messages. (view raw markdown for examples)
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.