Convert Tendermint Block Response #4922

RiccardoM · 2019-08-19T12:40:25Z

Converts a ResultBlock into a CosmosResultBlock having validator addresses represented as Bech32 addresses instead of Hex addresses.

Closes #3544

Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

…ddresses to the ValAddress type

…lock-response-conversion

RiccardoM · 2019-08-19T12:40:53Z

Should I write some tests for this? If so, which ones?

client/rpc/block.go

client/rpc/block_converter.go

go.mod

client/rpc/block_converter.go

client/rpc/block.go

client/rpc/block_converter.go

codecov · 2019-08-19T21:12:36Z

Codecov Report

Merging #4922 into master will decrease coverage by 0.57%.
The diff coverage is 98.68%.

@@            Coverage Diff             @@
##           master    #4922      +/-   ##
==========================================
- Coverage   54.79%   54.22%   -0.58%     
==========================================
  Files         307      312       +5     
  Lines       18296    18611     +315     
==========================================
+ Hits        10026    10092      +66     
- Misses       7494     7733     +239     
- Partials      776      786      +10

client/rpc/block.go

go.sum

client/rpc/block_converter.go

…ting the responses

client/rpc/block_converter.go

fedekunze

Overall LGTM. I'd like to see a simple test to check that the block addresses start with the given prefix. Eg:

require.True(t, strings.HasPrefix(valAddress, "cosmos1"))

client/rpc/block_converter.go

…sing comments

RiccardoM · 2019-10-16T10:01:30Z

I've reverted back the go.sum and go.mod file to the master version.

I've also added a simple test. However, I would like to have a test that you can make sure works properly. For this I would need

A default block resposne
The mapping between the hash address and its Bech32 representation

I'm also not able to reproduce what @alexanderbez said about the response not having the data inside it

alexanderbez · 2019-10-16T14:54:06Z

@RiccardoM I recommend you launch a local network and compare block responses pre and post changes. You'll see it's missing a bunch of the fields.

checkout gaia master
point sdk to your local via go mod edit ...
start localnet make clean install localnet-start
query for a block and compare....

RiccardoM · 2019-10-17T07:42:42Z

@alexanderbez thanks for the tip. I've updated the code to fix those errors.
I've also added the overriding of int, int64 and uint64 fields in order to preserve their serialization as strings.

alexanderbez

Looks pretty close @RiccardoM! Just general feedback on style. Also, starting to feel that it's a shame to introduce so much code just to change a single field type :-/

client/rpc/block_converter.go

…lexanderbez

…lexanderbez

RiccardoM · 2019-10-18T05:34:40Z

@alexanderbez Solved everything. It feels a bit like a shame, but I think it improves a lot the developer UX

RiccardoM · 2019-10-28T16:05:41Z

Ping @alexanderbez the issues should be solved. Can we merge this?

fedekunze

LGTM. I'd even say we could move the wrapper types into /types to provide an easy access to them for modules such as IBC.

Thoughts @alexanderbez @jackzampolin?

RiccardoM · 2019-11-07T06:25:14Z

Up @alexanderbez, @jackzampolin @fedekunze. Updates on this?

alessio

Few changes are required

alessio · 2019-11-07T06:30:38Z

client/rpc/block_converter_test.go

+	ctypes "github.com/tendermint/tendermint/rpc/core/types"
+)
+
+var defaultBlockJson = `{"block_meta":{"block_id":{"hash":"30CD3A9EF2082FF9F2575655E99E37ACC3936DC9E534ADF0BD7436C76258225C","parts":{"total":"1","hash":"463460DA73FA0AE441B6041BF2683AE625CE2D9FD290F4C86CD83D1A7FB9F439"}},"header":{"version":{"block":"10","app":"0"},"chain_id":"test-chain-RoCfX4","height":"16","time":"2019-10-16T09:47:21.054275229Z","num_txs":"0","total_txs":"0","last_block_id":{"hash":"168D140232DA3A59757658E257168B6BCE62024F574CA222C2D449BAB1AE4023","parts":{"total":"1","hash":"4E6D7A1C7DE6942470394C21B132A2D9F89B31908C3B6FA8C2AB96A87553B96E"}},"last_commit_hash":"3483509FCC8048496E9B85E1E4C90A3B6E5944F022FE3C0709096932A010F712","data_hash":"","validators_hash":"61E69204249E5EE6F777C0566040929276A1900CE665BCB11006CABF9A4ED9A3","next_validators_hash":"61E69204249E5EE6F777C0566040929276A1900CE665BCB11006CABF9A4ED9A3","consensus_hash":"048091BC7DDC283F77BFBF91D73C44DA58C3DF8A9CBC867405D8B7F3DAADA22F","app_hash":"070A8213F67F494F7D27A6296C29F0DF62DFA18FD37FB592D9761A44DEE96528","last_results_hash":"","evidence_hash":"","proposer_address":"E8B4AF895B301C3D7108CA93A0B9234E3A2A4B21"}},"block":{"header":{"version":{"block":"10","app":"0"},"chain_id":"test-chain-RoCfX4","height":"16","time":"2019-10-16T09:47:21.054275229Z","num_txs":"0","total_txs":"0","last_block_id":{"hash":"168D140232DA3A59757658E257168B6BCE62024F574CA222C2D449BAB1AE4023","parts":{"total":"1","hash":"4E6D7A1C7DE6942470394C21B132A2D9F89B31908C3B6FA8C2AB96A87553B96E"}},"last_commit_hash":"3483509FCC8048496E9B85E1E4C90A3B6E5944F022FE3C0709096932A010F712","data_hash":"","validators_hash":"61E69204249E5EE6F777C0566040929276A1900CE665BCB11006CABF9A4ED9A3","next_validators_hash":"61E69204249E5EE6F777C0566040929276A1900CE665BCB11006CABF9A4ED9A3","consensus_hash":"048091BC7DDC283F77BFBF91D73C44DA58C3DF8A9CBC867405D8B7F3DAADA22F","app_hash":"070A8213F67F494F7D27A6296C29F0DF62DFA18FD37FB592D9761A44DEE96528","last_results_hash":"","evidence_hash":"","proposer_address":"E8B4AF895B301C3D7108CA93A0B9234E3A2A4B21"},"data":{"txs":null},"evidence":{"evidence":null},"last_commit":{"block_id":{"hash":"168D140232DA3A59757658E257168B6BCE62024F574CA222C2D449BAB1AE4023","parts":{"total":"1","hash":"4E6D7A1C7DE6942470394C21B132A2D9F89B31908C3B6FA8C2AB96A87553B96E"}},"precommits":[{"type":2,"height":"15","round":"0","block_id":{"hash":"168D140232DA3A59757658E257168B6BCE62024F574CA222C2D449BAB1AE4023","parts":{"total":"1","hash":"4E6D7A1C7DE6942470394C21B132A2D9F89B31908C3B6FA8C2AB96A87553B96E"}},"timestamp":"2019-10-16T09:47:21.054275229Z","validator_address":"E8B4AF895B301C3D7108CA93A0B9234E3A2A4B21","validator_index":"0","signature":"eXB1NPmarUC5+2SCUQiWUCoGqtq0nrSn7giMAF/zWbfv3KmueE+1NoZ0CPLJAhBodz8Y8oL8xVy6ElA7J72kBw=="}]}}}`


Please avoid using global variables when writing test cases. This is currently used by one test case only

client/rpc/block_converter_test.go

…on' into riccardo/block-response-conversion

client/rpc/block_converter.go

client/rpc/block_converter_test.go

golangcibot · 2019-11-13T07:41:18Z

client/rpc/block_converter.go

+// does not respect the JSON stdlib embedding semantics.
+func (b Block) MarshalJSON() ([]byte, error) {
+	type blockJSON Block
+	return json.Marshal(blockJSON(b))


copylocks: call of blockJSON copies lock value: github.com/cosmos/cosmos-sdk/client/rpc.Block contains github.com/tendermint/tendermint/types.Block contains sync.Mutex (from govet)

We should probably return a reference to avoid mutex copying.

…lock-response-conversion

fedekunze · 2019-11-19T21:57:20Z

@alessio @alexanderbez mind reviewing again?

alexanderbez

Tested this out again. Looks good but I think two fields are still missing.

alexanderbez · 2019-12-02T15:26:42Z

client/rpc/block_converter_test.go

+
+	// test when the block is not nil
+	var block ctypes.ResultBlock
+	defaultBlockJSON := `{"block_meta":{"block_id":{"hash":"30CD3A9EF2082FF9F2575655E99E37ACC3936DC9E534ADF0BD7436C76258225C","parts":{"total":"1","hash":"463460DA73FA0AE441B6041BF2683AE625CE2D9FD290F4C86CD83D1A7FB9F439"}},"header":{"version":{"block":"10","app":"0"},"chain_id":"test-chain-RoCfX4","height":"16","time":"2019-10-16T09:47:21.054275229Z","num_txs":"0","total_txs":"0","last_block_id":{"hash":"168D140232DA3A59757658E257168B6BCE62024F574CA222C2D449BAB1AE4023","parts":{"total":"1","hash":"4E6D7A1C7DE6942470394C21B132A2D9F89B31908C3B6FA8C2AB96A87553B96E"}},"last_commit_hash":"3483509FCC8048496E9B85E1E4C90A3B6E5944F022FE3C0709096932A010F712","data_hash":"","validators_hash":"61E69204249E5EE6F777C0566040929276A1900CE665BCB11006CABF9A4ED9A3","next_validators_hash":"61E69204249E5EE6F777C0566040929276A1900CE665BCB11006CABF9A4ED9A3","consensus_hash":"048091BC7DDC283F77BFBF91D73C44DA58C3DF8A9CBC867405D8B7F3DAADA22F","app_hash":"070A8213F67F494F7D27A6296C29F0DF62DFA18FD37FB592D9761A44DEE96528","last_results_hash":"","evidence_hash":"","proposer_address":"E8B4AF895B301C3D7108CA93A0B9234E3A2A4B21"}},"block":{"header":{"version":{"block":"10","app":"0"},"chain_id":"test-chain-RoCfX4","height":"16","time":"2019-10-16T09:47:21.054275229Z","num_txs":"0","total_txs":"0","last_block_id":{"hash":"168D140232DA3A59757658E257168B6BCE62024F574CA222C2D449BAB1AE4023","parts":{"total":"1","hash":"4E6D7A1C7DE6942470394C21B132A2D9F89B31908C3B6FA8C2AB96A87553B96E"}},"last_commit_hash":"3483509FCC8048496E9B85E1E4C90A3B6E5944F022FE3C0709096932A010F712","data_hash":"","validators_hash":"61E69204249E5EE6F777C0566040929276A1900CE665BCB11006CABF9A4ED9A3","next_validators_hash":"61E69204249E5EE6F777C0566040929276A1900CE665BCB11006CABF9A4ED9A3","consensus_hash":"048091BC7DDC283F77BFBF91D73C44DA58C3DF8A9CBC867405D8B7F3DAADA22F","app_hash":"070A8213F67F494F7D27A6296C29F0DF62DFA18FD37FB592D9761A44DEE96528","last_results_hash":"","evidence_hash":"","proposer_address":"E8B4AF895B301C3D7108CA93A0B9234E3A2A4B21"},"data":{"txs":null},"evidence":{"evidence":null},"last_commit":{"block_id":{"hash":"168D140232DA3A59757658E257168B6BCE62024F574CA222C2D449BAB1AE4023","parts":{"total":"1","hash":"4E6D7A1C7DE6942470394C21B132A2D9F89B31908C3B6FA8C2AB96A87553B96E"}},"precommits":[{"type":2,"height":"15","round":"0","block_id":{"hash":"168D140232DA3A59757658E257168B6BCE62024F574CA222C2D449BAB1AE4023","parts":{"total":"1","hash":"4E6D7A1C7DE6942470394C21B132A2D9F89B31908C3B6FA8C2AB96A87553B96E"}},"timestamp":"2019-10-16T09:47:21.054275229Z","validator_address":"E8B4AF895B301C3D7108CA93A0B9234E3A2A4B21","validator_index":"0","signature":"eXB1NPmarUC5+2SCUQiWUCoGqtq0nrSn7giMAF/zWbfv3KmueE+1NoZ0CPLJAhBodz8Y8oL8xVy6ElA7J72kBw=="}]}}}`


Can we use block JSON that has all fields populated (i.e. txs and evidence). I submitted a tx (using this branch) and when querying the block, I don't see the tx under block.txs. I bet the same will hold true for block.evidence.

So ideally we have:

// ... convertedBlock := ConvertBlockResult(&block) expectedOut := `{...}` bz, err := cdc.MarshalJSON(convertedBlock) require.NoError(t, err) require.Equal(t, expectedOut, string(bz))

alexanderbez · 2019-12-02T15:29:32Z

client/rpc/block_converter.go

+// does not respect the JSON stdlib embedding semantics.
+func (b Block) MarshalJSON() ([]byte, error) {
+	type blockJSON Block
+	return json.Marshal(blockJSON(b))


We should probably return a reference to avoid mutex copying.

stale · 2020-01-01T16:17:40Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

RiccardoM added 2 commits August 19, 2019 14:37

Added the conversion of the block in order to convert HEX validator a…

9c8bcba

…ddresses to the ValAddress type

Merge branch 'master' of github.com:cosmos/cosmos-sdk into riccardo/b…

37c46f2

…lock-response-conversion

RiccardoM requested review from alessio, alexanderbez, fedekunze, jackzampolin and rigelrozanski as code owners August 19, 2019 12:40

golangcibot reviewed Aug 19, 2019

View reviewed changes

client/rpc/block.go Outdated Show resolved Hide resolved

client/rpc/block_converter.go Outdated Show resolved Hide resolved

client/rpc/block_converter.go Outdated Show resolved Hide resolved

client/rpc/block_converter.go Outdated Show resolved Hide resolved

alexanderbez added R4R labels Aug 19, 2019

alexanderbez suggested changes Aug 19, 2019

View reviewed changes

client/rpc/block_converter.go Outdated Show resolved Hide resolved

go.mod Outdated Show resolved Hide resolved

client/rpc/block_converter.go Outdated Show resolved Hide resolved

tnachen reviewed Aug 19, 2019

View reviewed changes

client/rpc/block.go Outdated Show resolved Hide resolved

tac0turtle reviewed Aug 19, 2019

View reviewed changes

client/rpc/block_converter.go Outdated Show resolved Hide resolved

RiccardoM added 3 commits August 19, 2019 22:46

Removed mutex

604f494

Undo go mod change

40604b4

Removed Cosmos* type prefix

4439a72

Sorted imports using goimports instead of gofmt

499818d

alexanderbez suggested changes Aug 20, 2019

View reviewed changes

client/rpc/block.go Outdated Show resolved Hide resolved

go.sum Outdated Show resolved Hide resolved

client/rpc/block_converter.go Show resolved Hide resolved

RiccardoM added 3 commits August 21, 2019 10:38

Applied suggestion to conversion call

e29aba1

Undo changed in go.sum

6934f17

Used type wrappers instead of re defining the whole type while conver…

93a8526

…ting the responses

golangcibot reviewed Aug 21, 2019

View reviewed changes

client/rpc/block_converter.go Show resolved Hide resolved

client/rpc/block_converter.go Show resolved Hide resolved

alexanderbez suggested changes Aug 21, 2019

View reviewed changes

client/rpc/block_converter.go Show resolved Hide resolved

client/rpc/block_converter.go Show resolved Hide resolved

Added // nolint: structtag

faa3f2c

fedekunze suggested changes Aug 23, 2019

View reviewed changes

client/rpc/block_converter.go Show resolved Hide resolved

client/rpc/block_converter.go Outdated Show resolved Hide resolved

tac0turtle reviewed Aug 23, 2019

View reviewed changes

client/rpc/block_converter.go Show resolved Hide resolved

RiccardoM added 2 commits August 26, 2019 07:52

Removed useless comments and variables while converting a block

69fd89c

Unified how different types are treated (encapsulation) and added mis…

909faa0

…sing comments

Fixed the missing of some fields

b520774

RiccardoM requested a review from alexanderbez October 17, 2019 07:42

alexanderbez added 2 commits October 17, 2019 09:44

Merge branch 'master' into riccardo/block-response-conversion

d8daeab

Merge branch 'master' into riccardo/block-response-conversion

8e04baf

alexanderbez suggested changes Oct 17, 2019

View reviewed changes

Updated the comments and removed some useless variables as said by @a…

3a4d706

…lexanderbez

RiccardoM requested a review from alexanderbez October 18, 2019 05:33

RiccardoM requested a review from fedekunze October 18, 2019 05:35

fedekunze approved these changes Oct 28, 2019

View reviewed changes

Merge branch 'master' into riccardo/block-response-conversion

8a24e6d

alessio suggested changes Nov 7, 2019

View reviewed changes

RiccardoM added 2 commits November 13, 2019 08:38

Removed global test variable and implemented nil test

dbeb759

Merge remote-tracking branch 'origin/riccardo/block-response-conversi…

37f2585

…on' into riccardo/block-response-conversion

golangcibot reviewed Nov 13, 2019

View reviewed changes

RiccardoM added 2 commits November 13, 2019 08:45

Merge branch 'master' of github.com:cosmos/cosmos-sdk into riccardo/b…

f241029

…lock-response-conversion

Renamed variables to solve Golint errors

1f9e9db

RiccardoM requested a review from alessio November 13, 2019 09:51

Merge branch 'master' into riccardo/block-response-conversion

56a37b9

Merge branch 'master' into riccardo/block-response-conversion

1815c3a

alexanderbez suggested changes Dec 2, 2019

View reviewed changes

stale bot added the stale label Jan 1, 2020

stale bot closed this Jan 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert Tendermint Block Response #4922

Convert Tendermint Block Response #4922

RiccardoM commented Aug 19, 2019 •

edited

Loading

RiccardoM commented Aug 19, 2019

codecov bot commented Aug 19, 2019 •

edited

Loading

fedekunze left a comment •

edited

Loading

RiccardoM commented Oct 16, 2019 •

edited

Loading

alexanderbez commented Oct 16, 2019 •

edited

Loading

RiccardoM commented Oct 17, 2019

alexanderbez left a comment

RiccardoM commented Oct 18, 2019

RiccardoM commented Oct 28, 2019

fedekunze left a comment

RiccardoM commented Nov 7, 2019 •

edited

Loading

alessio left a comment

alessio Nov 7, 2019

RiccardoM Nov 13, 2019

golangcibot Nov 13, 2019

alexanderbez Dec 2, 2019

fedekunze commented Nov 19, 2019

alexanderbez left a comment

alexanderbez Dec 2, 2019

alexanderbez Dec 2, 2019

stale bot commented Jan 1, 2020

Convert Tendermint Block Response #4922

Convert Tendermint Block Response #4922

Conversation

RiccardoM commented Aug 19, 2019 • edited Loading

RiccardoM commented Aug 19, 2019

codecov bot commented Aug 19, 2019 • edited Loading

Codecov Report

fedekunze left a comment • edited Loading

Choose a reason for hiding this comment

RiccardoM commented Oct 16, 2019 • edited Loading

alexanderbez commented Oct 16, 2019 • edited Loading

RiccardoM commented Oct 17, 2019

alexanderbez left a comment

Choose a reason for hiding this comment

RiccardoM commented Oct 18, 2019

RiccardoM commented Oct 28, 2019

fedekunze left a comment

Choose a reason for hiding this comment

RiccardoM commented Nov 7, 2019 • edited Loading

alessio left a comment

Choose a reason for hiding this comment

alessio Nov 7, 2019

Choose a reason for hiding this comment

RiccardoM Nov 13, 2019

Choose a reason for hiding this comment

golangcibot Nov 13, 2019

Choose a reason for hiding this comment

alexanderbez Dec 2, 2019

Choose a reason for hiding this comment

fedekunze commented Nov 19, 2019

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez Dec 2, 2019

Choose a reason for hiding this comment

alexanderbez Dec 2, 2019

Choose a reason for hiding this comment

stale bot commented Jan 1, 2020

RiccardoM commented Aug 19, 2019 •

edited

Loading

codecov bot commented Aug 19, 2019 •

edited

Loading

fedekunze left a comment •

edited

Loading

RiccardoM commented Oct 16, 2019 •

edited

Loading

alexanderbez commented Oct 16, 2019 •

edited

Loading

RiccardoM commented Nov 7, 2019 •

edited

Loading