refactor auths broadcast cmd in alignment with #6216 #6713

clevinson · 2020-07-14T03:11:05Z

Description

Changes:

add protobuf definitions for TxResponse and ABCIMessageLog, for use in x/auth's broadcast commands
update corresponding functions to return TxResponse by reference instead of by value
update UnpackInterfaces implementation of TxResponse to not unpack Tx fields when nil

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.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

codecov · 2020-07-14T03:15:09Z

Codecov Report

Merging #6713 into master will decrease coverage by 0.03%.
The diff coverage is 41.37%.

@@            Coverage Diff             @@
##           master    #6713      +/-   ##
==========================================
- Coverage   59.87%   59.83%   -0.04%     
==========================================
  Files         513      513              
  Lines       31470    31483      +13     
==========================================
- Hits        18842    18838       -4     
- Misses      11192    11209      +17     
  Partials     1436     1436

…act-broadcast-cmd

clevinson · 2020-07-16T05:20:31Z

I think i've covered all the components I need to pull out from #6216, but some integration tests are still failing and i'm not exactly sure why.

Getting a invalid memory address or nil pointer dereference error on all failing tests, which are calling UnpackInterfaces on the TxResponse to unpack the underlying Tx. Is there some work that I need to do involving RegisterInterface that i'm missing here @aaronc ?

amaury1093 · 2020-07-16T14:38:26Z

I'm maybe missing some context here. iiuc @clevinson you're tackling the "migrate x/auth commands to use TxGenerator and SignatureV2 -> broadcast" item in #6213, but afaict, it has been already done?

https://github.com/cosmos/cosmos-sdk/pull/6391/files#diff-95109ccb95ab5a6a697971116e43119bR39

edit: indeed the other commands still need to be converted, but broadcast seems done to me.

clevinson · 2020-07-16T15:01:51Z

@amaurymartiny yes indeed there was some work on actually migrating the broadcast command that was done, but one of hte larger missing pieces was that TxResponse needed to get converted to a protobuf message (along with ABCIMessageLog), and this was deemed part of the broadcast migration as there weren't any other cli commands using it.

After switching that over, there were a number of minor changes to properly handle the proto TxResponse, namely:

TxResponse packs the tx inside an Any and needs some custom UnsafePackAny capabilities that were used in Implement SIGN_MODE_DIRECT #6216
a few other functions made use of TxResponse that I migrated over:
- SearchTxResponse
- subsequently some components of gov's queriers make use of SearchTxResponse here

…Interfaces

clevinson · 2020-07-17T07:38:00Z

I fixed the integration tests by updating TxResponse's implementation of UnpackInterfaces to not call into UnpackAny if the Tx inside of TxResponse is nil. This is similar to the approach I saw for implementing UnpackInterfaces on Tx here.

An alternative that I was less sure about could have involved calling UnsafePackAny to pack the nil value inside an any with a cachedValue = nil, so that UnpackAny could be called on TxResponse regardless of whether an underlying Tx is has a value. But I didn't see that as a convention, and it seemed a bit messy.

Also, just flagging that i'm not sure what breakout issue from this epic will take care of adding the MarshalAny and UnmarshalAny interfaces that were done initially as part of #6216 (see here). Should that be taken care of in this PR, or is it being accomplished by some other component of #6213 ?

…act-broadcast-cmd

aaronc · 2020-07-17T17:00:20Z

I fixed the integration tests by updating TxResponse's implementation of UnpackInterfaces to not call into UnpackAny if the Tx inside of TxResponse is nil. This is similar to the approach I saw for implementing UnpackInterfaces on Tx here.

An alternative that I was less sure about could have involved calling UnsafePackAny to pack the nil value inside an any with a cachedValue = nil, so that UnpackAny could be called on TxResponse regardless of whether an underlying Tx is has a value. But I didn't see that as a convention, and it seemed a bit messy.

Also, just flagging that i'm not sure what breakout issue from this epic will take care of adding the MarshalAny and UnmarshalAny interfaces that were done initially as part of #6216 (see here). Should that be taken care of in this PR, or is it being accomplished by some other component of #6213 ?

Is the Marshal/UnmarshalAny needed for this PR?

aaronc · 2020-07-17T17:09:55Z

proto/cosmos/cosmos.proto

+message TxResponse {
+  option (gogoproto.goproto_getters) = false;
+
+  int64 height = 1;
+  string txhash = 2 [(gogoproto.customname) = "TxHash"];
+  string codespace = 3;
+  uint32 code = 4;
+  string data = 5;
+  string raw_log = 6;
+  repeated ABCIMessageLog logs = 7 [(gogoproto.castrepeated) = "ABCIMessageLogs", (gogoproto.nullable) = false];
+  string info = 8;
+  int64 gas_wanted = 9;
+  int64 gas_used = 10;
+  google.protobuf.Any tx = 11;
+  string timestamp = 12;
+}


I do want to note that I do think this stuff should live in the root cosmos proto package. I'm not sure where it should go but I don't feel comfortable with it here. Any thoughts?

I'd also like to take out the other Result, SimulationResponse, etc. stuff and leave just Coin, DecCoin & Int/DecProto preferably before 0.40 is done.

This makes sense. I can tackle this as another PR.

aaronc

LGTM. Would be nice to clean up cosmos.proto, but maybe as a separate PR.

clevinson · 2020-07-17T17:12:38Z

Is the Marshal/UnmarshalAny needed for this PR?

Nope, not necessary for this PR, just flagging that I want to make sure it is covered somewhere to be tackled in the #6213 bullet list if we want to include it in 0.40

aaronc · 2020-07-17T17:16:55Z

Is the Marshal/UnmarshalAny needed for this PR?

Nope, not necessary for this PR, just flagging that I want to make sure it is covered somewhere to be tackled in the #6213 bullet list if we want to include it in 0.40

The MarshalAny stuff is more relevant for #6190

alexanderbez · 2020-07-17T18:22:14Z

types/result.go

 func NewABCIMessageLog(i uint16, log string, events Events) ABCIMessageLog {
 	return ABCIMessageLog{
-		MsgIndex: i,
+		MsgIndex: uint32(i),


There shouldn't be a need to cast here, rather the function should just accept a uint32.

alexanderbez · 2020-07-17T18:24:17Z

codec/types/any.go

+// in the case of a packing failure, keeps the cached value. This should only
+// be used in situations where compatibility is needed with amino. Amino-only
+// values can safely be packed using this method when they will only be
+// marshaled with amino and not protobuf


Missing puncuation.

* refactor auths broadcast cmd in alignment with cosmos#6216 * add TxResponse proto definition, and related refactoring * re-run make proto-gen, updating staking.pb.go * cleanup TxResponse tests to handle nil return values * properly handle nil Tx value in TxResponse's implementation of UnpackInterfaces Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

refactor auths broadcast cmd in alignment with #6216

7a305ab

Merge remote-tracking branch 'origin/master' into clevinson/6216-extr…

0a440d5

…act-broadcast-cmd

clevinson mentioned this pull request Jul 15, 2020

Implement SIGN_MODE_DIRECT #6216

Closed

clevinson added 4 commits July 15, 2020 20:26

add TxResponse proto definition, and related refactoring

a74fcec

Merge remote-tracking branch 'origin/master' into clevinson/6216-extr…

03eaa86

…act-broadcast-cmd

re-run make proto-gen, updating staking.pb.go

ce04d9f

cleanup TxResponse tests to handle nil return values

5f3e036

aaronc requested review from sahith-narahari, amaury1093, anilcse, atheeshp and blushi July 16, 2020 13:45

properly handle nil Tx value in TxResponse's implementation of Unpack…

ffecb37

…Interfaces

clevinson added 2 commits July 17, 2020 00:38

Merge remote-tracking branch 'origin/master' into clevinson/6216-extr…

d958786

…act-broadcast-cmd

Merge branch 'master' into clevinson/6216-extract-broadcast-cmd

fbe58cc

clevinson added this to the v0.40 [Stargate] milestone Jul 17, 2020

clevinson marked this pull request as ready for review July 17, 2020 15:19

clevinson requested review from aaronc, alessio, alexanderbez, fedekunze and jgimeno as code owners July 17, 2020 15:19

fedekunze approved these changes Jul 17, 2020

View reviewed changes

Merge branch 'master' into clevinson/6216-extract-broadcast-cmd

47f741d

aaronc reviewed Jul 17, 2020

View reviewed changes

aaronc approved these changes Jul 17, 2020

View reviewed changes

aaronc added the A:automerge Automatically merge PR once all prerequisites pass. label Jul 17, 2020

mergify bot merged commit 32de79d into master Jul 17, 2020

mergify bot deleted the clevinson/6216-extract-broadcast-cmd branch July 17, 2020 17:17

alexanderbez reviewed Jul 17, 2020

View reviewed changes

clevinson mentioned this pull request Jul 18, 2020

Small updates to #6713 #6777

Merged

9 tasks

aaronc mentioned this pull request Jul 20, 2020

Proto Any Tx migration #6213

Closed

27 tasks

clevinson mentioned this pull request Jul 23, 2020

Audit Protobuf package names & update RegisterInterface calls #6822

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor auths broadcast cmd in alignment with #6216 #6713

refactor auths broadcast cmd in alignment with #6216 #6713

clevinson commented Jul 14, 2020 •

edited

Loading

codecov bot commented Jul 14, 2020 •

edited

Loading

clevinson commented Jul 16, 2020

amaury1093 commented Jul 16, 2020 •

edited

Loading

clevinson commented Jul 16, 2020 •

edited

Loading

clevinson commented Jul 17, 2020 •

edited

Loading

aaronc commented Jul 17, 2020

aaronc Jul 17, 2020 •

edited

Loading

clevinson Jul 17, 2020 •

edited

Loading

aaronc Jul 17, 2020

aaronc left a comment

clevinson commented Jul 17, 2020

aaronc commented Jul 17, 2020

alexanderbez Jul 17, 2020

alexanderbez Jul 17, 2020

refactor auths broadcast cmd in alignment with #6216 #6713

refactor auths broadcast cmd in alignment with #6216 #6713

Conversation

clevinson commented Jul 14, 2020 • edited Loading

Description

codecov bot commented Jul 14, 2020 • edited Loading

Codecov Report

clevinson commented Jul 16, 2020

amaury1093 commented Jul 16, 2020 • edited Loading

clevinson commented Jul 16, 2020 • edited Loading

clevinson commented Jul 17, 2020 • edited Loading

aaronc commented Jul 17, 2020

aaronc Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

clevinson Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

aaronc Jul 17, 2020

Choose a reason for hiding this comment

aaronc left a comment

Choose a reason for hiding this comment

clevinson commented Jul 17, 2020

aaronc commented Jul 17, 2020

alexanderbez Jul 17, 2020

Choose a reason for hiding this comment

alexanderbez Jul 17, 2020

Choose a reason for hiding this comment

clevinson commented Jul 14, 2020 •

edited

Loading

codecov bot commented Jul 14, 2020 •

edited

Loading

amaury1093 commented Jul 16, 2020 •

edited

Loading

clevinson commented Jul 16, 2020 •

edited

Loading

clevinson commented Jul 17, 2020 •

edited

Loading

aaronc Jul 17, 2020 •

edited

Loading

clevinson Jul 17, 2020 •

edited

Loading