refactor: change tmblock to sdkblock #11390

atheeshp · 2022-03-16T09:43:04Z

Description

Closes: #3544

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...

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 state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

…-string

amaury1093 · 2022-03-24T13:10:11Z

proto/cosmos/base/tendermint/v1beta1/query.proto

@@ -85,7 +85,7 @@ message GetBlockByHeightRequest {
 // GetBlockByHeightResponse is the response type for the Query/GetBlockByHeight RPC method.
 message GetBlockByHeightResponse {
  .tendermint.types.BlockID block_id = 1;
-  .tendermint.types.Block   block    = 2;
+  Block                     block    = 2;


We unfortunately can't do that without bumping to v1beta2.

Can we instead add a new field in the Response message?

amaury1093

blocking for now because it is proto breaking

client/grpc/tmservice/util.go

amaury1093 · 2022-03-24T15:37:54Z

@atheeshp Do we also have an endpoint for SDK blocks? maybe we can just put the bech32 addresses in that block.

tac0turtle · 2022-03-25T12:23:32Z

can we reuse https://github.com/cosmos/cosmos-sdk/blob/master/proto/cosmos/tx/v1beta1/service.proto#L40 for this?

amaury1093 · 2022-03-29T12:11:38Z

proto/cosmos/base/tendermint/v1beta1/query.proto

@@ -94,7 +94,7 @@ message GetLatestBlockRequest {}
 // GetLatestBlockResponse is the response type for the Query/GetLatestBlock RPC method.
 message GetLatestBlockResponse {
  .tendermint.types.BlockID block_id = 1;
-  .tendermint.types.Block   block    = 2;
+  Block                     block    = 2;


So we can't change this, neither here nor on the TxService as marko suggested here #11390 (comment), because both would be proto-breaking.

What we can do is simply add a new proposer string field to either or both of these 2 response types.

The alternative is to add a new field sdk_block on either or both of these 2 response types, which has the new Block type.

amaury1093 · 2022-03-30T14:38:40Z

Punting this to later, when 0.46 QA is done.

tac0turtle · 2022-06-13T18:24:29Z

@atheeshp should we assign this to someone else or are you fine to finish it

atheeshp · 2022-06-14T05:17:57Z

@atheeshp should we assign this to someone else or are you fine to finish it

I can continue this, this is halted due to 0.46.

… ap/byte-to-string

amaury1093

Overall lgtm, let's also add a changelog. I propose to deprecate the usage of the TM block type, and just use ours.

amaury1093 · 2022-07-11T10:30:58Z

proto/cosmos/base/tendermint/v1beta1/query.proto

@@ -100,6 +101,7 @@ message GetBlockByHeightRequest {
 message GetBlockByHeightResponse {
  .tendermint.types.BlockID block_id = 1;
  .tendermint.types.Block   block    = 2;
+  Block                     sdk_block    = 3;


Let's add a comment "Since", and also what's the difference between this and block

There is a field proposer_address with type bytes in tm block, and in sdk block it is changed to string

proto/cosmos/base/tendermint/v1beta1/query.proto

proto/cosmos/base/tendermint/v1beta1/types.proto

…tring

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

codecov · 2022-07-12T07:24:17Z

Codecov Report

Merging #11390 (51663c9) into main (203d563) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main   #11390      +/-   ##
==========================================
- Coverage   65.32%   65.27%   -0.05%     
==========================================
  Files         693      694       +1     
  Lines       71823    71851      +28     
==========================================
- Hits        46916    46899      -17     
- Misses      22268    22314      +46     
+ Partials     2639     2638       -1

Impacted Files	Coverage Δ
client/grpc/tmservice/service.go	`0.00% <0.00%> (ø)`
client/grpc/tmservice/util.go	`0.00% <0.00%> (ø)`
x/distribution/simulation/operations.go	`80.85% <0.00%> (-9.58%)`	⬇️
crypto/keys/internal/ecdsa/privkey.go	`83.01% <0.00%> (+1.88%)`	⬆️

…tring

… ap/byte-to-string

tac0turtle

LGTM, should we backport this?

amaury1093

LGTM, I added some suggestions for comments to make it clear what we're doing here.

proto/cosmos/base/tendermint/v1beta1/query.proto

proto/cosmos/base/tendermint/v1beta1/types.proto

amaury1093 · 2022-07-13T07:29:43Z

proto/cosmos/base/tendermint/v1beta1/query.proto

+  .tendermint.types.Block   block    = 2 [deprecated = true];
+
+  // since: v0.47
+  Block sdk_block = 3;


Not sure about the sdk_block name. Any other ideas, maybe @alexanderbez ?

I guess the main issue with naming here is no matter what in the future we will rename it to block instead of whatever its called. It might be fine for now then

amaury1093

thanks @atheeshp

## Description Closes: #3544 --- ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable) (cherry picked from commit e89db04) # Conflicts: # api/cosmos/base/tendermint/v1beta1/query.pulsar.go # client/grpc/tmservice/query.pb.go

* refactor: change tmblock to sdkblock (#11390) ## Description Closes: #3544 --- ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable) (cherry picked from commit e89db04) # Conflicts: # api/cosmos/base/tendermint/v1beta1/query.pulsar.go # client/grpc/tmservice/query.pb.go * resolve conflicts * remove api/ Co-authored-by: atheeshp <59333759+atheeshp@users.noreply.github.com> Co-authored-by: atheesh <atheesh@vitwit.com>

atheeshp added 3 commits March 15, 2022 16:01

add proto

127aed0

refactor: change tmblock to sdkblock

bd302b2

Merge branch 'master' of github.com:cosmos/cosmos-sdk into ap/byte-to…

c5389e6

…-string

github-actions bot added the C:CLI label Mar 16, 2022

atheeshp and others added 3 commits March 16, 2022 15:42

proto updates

462d36c

update tests

b61b909

Merge branch 'master' into ap/byte-to-string

782467b

atheeshp marked this pull request as ready for review March 16, 2022 11:35

atheeshp requested review from aaronc and alexanderbez as code owners March 16, 2022 11:35

atheeshp requested review from amaury1093, blushi, likhita-809 and tac0turtle March 16, 2022 11:35

amaury1093 assigned amaury1093 and likhita-809 Mar 17, 2022

Merge branch 'master' into ap/byte-to-string

1442415

amaury1093 reviewed Mar 24, 2022

View reviewed changes

amaury1093 suggested changes Mar 24, 2022

View reviewed changes

alexanderbez reviewed Mar 24, 2022

View reviewed changes

client/grpc/tmservice/util.go Outdated Show resolved Hide resolved

client/grpc/tmservice/util.go Outdated Show resolved Hide resolved

amaury1093 reviewed Mar 29, 2022

View reviewed changes

amaury1093 marked this pull request as draft March 30, 2022 14:38

tac0turtle added the S:blocked Status: Blocked label May 9, 2022

tac0turtle removed the S:blocked Status: Blocked label Jun 14, 2022

tac0turtle added the T:Sprint label Jun 21, 2022

atheeshp added 2 commits July 7, 2022 12:05

Merge branch 'ap/byte-to-string' of github.com:cosmos/cosmos-sdk into…

bbcb88a

… ap/byte-to-string

review changes

294b354

atheeshp requested a review from amaury1093 July 7, 2022 06:37

amaury1093 reviewed Jul 11, 2022

View reviewed changes

atheeshp and others added 5 commits July 12, 2022 12:23

Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/byte-to-s…

f35735c

…tring

review changes

0c0001a

proto-all

db79bd0

Merge branch 'main' into ap/byte-to-string

4f39160

Update client/grpc/tmservice/util.go

8b54dfe

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

atheeshp requested review from amaury1093 and alexanderbez July 12, 2022 07:08

atheeshp marked this pull request as ready for review July 12, 2022 07:08

atheeshp requested a review from a team as a code owner July 12, 2022 07:08

atheeshp and others added 4 commits July 13, 2022 12:23

Merge branch 'main' into ap/byte-to-string

aaf035f

Merge branch 'main' of github.com:cosmos/cosmos-sdk into ap/byte-to-s…

6faa7c8

…tring

add changelo

7c8935f

Merge branch 'ap/byte-to-string' of github.com:cosmos/cosmos-sdk into…

045a0e5

… ap/byte-to-string

tac0turtle approved these changes Jul 13, 2022

View reviewed changes

amaury1093 reviewed Jul 13, 2022

View reviewed changes

amaury1093 added the backport/0.46.x PR scheduled for inclusion in the v0.46's next stable release label Jul 13, 2022

review changes

51663c9

amaury1093 approved these changes Jul 13, 2022

View reviewed changes

amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Jul 13, 2022

mergify bot merged commit e89db04 into main Jul 13, 2022

mergify bot deleted the ap/byte-to-string branch July 13, 2022 09:47

mergify bot mentioned this pull request Jul 13, 2022

refactor: change tmblock to sdkblock (backport #11390) #12549

Merged

atheeshp mentioned this pull request Dec 6, 2022

Bug: sdk_block translated proposer address is wrong #14079

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: change tmblock to sdkblock #11390

refactor: change tmblock to sdkblock #11390

atheeshp commented Mar 16, 2022

amaury1093 Mar 24, 2022

amaury1093 left a comment

amaury1093 commented Mar 24, 2022

tac0turtle commented Mar 25, 2022

amaury1093 Mar 29, 2022

amaury1093 commented Mar 30, 2022

tac0turtle commented Jun 13, 2022

atheeshp commented Jun 14, 2022 •

edited

Loading

amaury1093 left a comment

amaury1093 Jul 11, 2022

atheeshp Jul 12, 2022

codecov bot commented Jul 12, 2022 •

edited

Loading

tac0turtle left a comment

amaury1093 left a comment

amaury1093 Jul 13, 2022

tac0turtle Jul 13, 2022

amaury1093 left a comment

refactor: change tmblock to sdkblock #11390

refactor: change tmblock to sdkblock #11390

Conversation

atheeshp commented Mar 16, 2022

Description

Author Checklist

Reviewers Checklist

amaury1093 Mar 24, 2022

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 commented Mar 24, 2022

tac0turtle commented Mar 25, 2022

amaury1093 Mar 29, 2022

Choose a reason for hiding this comment

amaury1093 commented Mar 30, 2022

tac0turtle commented Jun 13, 2022

atheeshp commented Jun 14, 2022 • edited Loading

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 Jul 11, 2022

Choose a reason for hiding this comment

atheeshp Jul 12, 2022

Choose a reason for hiding this comment

codecov bot commented Jul 12, 2022 • edited Loading

Codecov Report

tac0turtle left a comment

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 Jul 13, 2022

Choose a reason for hiding this comment

tac0turtle Jul 13, 2022

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

atheeshp commented Jun 14, 2022 •

edited

Loading

codecov bot commented Jul 12, 2022 •

edited

Loading