Skip to content

proto: add proto files for ibc unblock (#4853)#4906

Merged
tac0turtle merged 1 commit intorc2/v0.33.5from
marko/proto_ibc
May 27, 2020
Merged

proto: add proto files for ibc unblock (#4853)#4906
tac0turtle merged 1 commit intorc2/v0.33.5from
marko/proto_ibc

Conversation

@tac0turtle
Copy link
Contributor

Description

these proto files are meant to help unblock ibc in their quest of migrating the ibc module to proto.

Closes: #XXX

@auto-comment
Copy link

auto-comment bot commented May 27, 2020

👋 Thanks for creating a PR!

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.

  • Wrote tests
  • Updated CHANGELOG_PENDING.md
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments
  • Re-reviewed Files changed in the Github PR explorer
  • Applied Appropriate Labels

Thank you for your contribution to Tendermint! 🚀

these proto files are meant to help unblock ibc in their quest of migrating the ibc module to proto.

Closes: #XXX
@tac0turtle tac0turtle marked this pull request as ready for review May 27, 2020 11:02
@tac0turtle tac0turtle requested review from melekes and tessr as code owners May 27, 2020 11:02
@tac0turtle tac0turtle self-assigned this May 27, 2020
@tac0turtle tac0turtle requested a review from fedekunze May 27, 2020 11:02
@codecov-commenter
Copy link

Codecov Report

Merging #4906 into rc2/v0.33.5 will decrease coverage by 0.61%.
The diff coverage is 100.00%.

@@               Coverage Diff               @@
##           rc2/v0.33.5    #4906      +/-   ##
===============================================
- Coverage        66.19%   65.58%   -0.62%     
===============================================
  Files              247      172      -75     
  Lines            22300    14305    -7995     
===============================================
- Hits             14762     9382    -5380     
+ Misses            6372     4159    -2213     
+ Partials          1166      764     -402     
Impacted Files Coverage Δ
libs/bits/bit_array.go 81.14% <100.00%> (+1.06%) ⬆️
p2p/pex/errors.go 11.11% <0.00%> (-11.12%) ⬇️
blockchain/v2/routine.go 77.61% <0.00%> (-2.99%) ⬇️
blockchain/v0/reactor.go 73.91% <0.00%> (-1.74%) ⬇️
p2p/pex/pex_reactor.go 83.84% <0.00%> (-0.56%) ⬇️
p2p/pex/addrbook.go 71.89% <0.00%> (-0.44%) ⬇️
blockchain/v0/pool.go 78.34% <0.00%> (-0.32%) ⬇️
p2p/conn/connection.go 80.71% <0.00%> (-0.26%) ⬇️
store/codec.go
types/events.go
... and 78 more

Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

ACK, minor comments.

Comment on lines +275 to +278
{"success empty", &BitArray{}, true},
{"success", NewBitArray(1), true},
{"success", NewBitArray(2), true},
{"negative", NewBitArray(-1), false},
Copy link
Contributor

Choose a reason for hiding this comment

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

add a test with NewBitArray(0)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will make the change to this test on master, that work?

if commit.Round < 0 {
return errors.New("negative Round")
}
if commit.Height >= 1 {
Copy link
Contributor

@fedekunze fedekunze May 27, 2020

Choose a reason for hiding this comment

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

nit, use inverse for better readability

	if commit.Height < 1 {
		return nil
	}
	if commit.BlockID.IsZero() {
		return errors.New("commit cannot be for nil block")
	}
...

Copy link
Contributor

Choose a reason for hiding this comment

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

no, you can't do that. commit.Height < 1 does not make the whole commit valid

@tac0turtle tac0turtle merged commit 8c35827 into rc2/v0.33.5 May 27, 2020
@tac0turtle tac0turtle deleted the marko/proto_ibc branch May 27, 2020 15:33
tessr pushed a commit that referenced this pull request May 28, 2020
these proto files are meant to help unblock ibc in their quest of migrating the ibc module to proto.
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.

4 participants