Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

types: Refactor EventAttribute #6408

Merged
merged 9 commits into from
Apr 30, 2021
Merged

types: Refactor EventAttribute #6408

merged 9 commits into from
Apr 30, 2021

Conversation

alexanderbez
Copy link
Contributor

closes: #6403

@alexanderbez alexanderbez marked this pull request as ready for review April 29, 2021 20:21
@alexanderbez alexanderbez added C:abci Component: Application Blockchain Interface C:events Component: Events T:breaking Type: Breaking Change labels Apr 29, 2021
tac0turtle
tac0turtle approved these changes Apr 29, 2021
View reviewed changes
Copy link
Contributor

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

LGTM.

Are we fine with this breaking change for 0.35?

@ethanfrey
Copy link
Contributor

FYI it is less breaking than it appears.

https://developers.google.com/protocol-buffers/docs/proto#updating

string and bytes are compatible as long as the bytes are valid UTF-8.

So anyone who didn't have trouble earlier (passing in utf8) will not even notice the change and can use 0.35 without updating their protobuf defs abci side

@alexanderbez alexanderbez added S:automerge Automatically merge PR when requirements pass and removed T:breaking Type: Breaking Change labels Apr 30, 2021
@codecov
Copy link

codecov bot commented Apr 30, 2021 •
edited
Loading

Codecov Report

Merging #6408 (51aa336) into master (be2ac87) will increase coverage by 0.02%.
The diff coverage is 75.86%.

❗ Current head 51aa336 differs from pull request most recent head 0fc5348. Consider uploading reports for the commit 0fc5348 to get more accurate results

@@            Coverage Diff             @@
##           master    #6408      +/-   ##
==========================================
+ Coverage   60.93%   60.96%   +0.02%     
==========================================
  Files         283      283              
  Lines       27012    26993      -19     
==========================================
- Hits        16461    16457       -4     
+ Misses       8829     8824       -5     
+ Partials     1722     1712      -10
Impacted Files Coverage Δ
config/config.go 74.53% <ø> (+0.07%) ⬆️
config/toml.go 59.09% <ø> (ø)
mempool/clist_mempool.go 82.82% <ø> (+1.68%) ⬆️
mempool/mempool.go 81.25% <ø> (ø)
node/node.go 57.40% <ø> (+0.33%) ⬆️
test/e2e/generator/main.go 0.00% <0.00%> (ø)
state/execution.go 70.07% <72.72%> (+0.28%) ⬆️
abci/example/kvstore/kvstore.go 68.75% <100.00%> (ø)
state/indexer/block/kv/kv.go 50.48% <100.00%> (ø)
state/indexer/tx/kv/kv.go 60.46% <100.00%> (ø)
... and 14 more

@alexanderbez alexanderbez merged commit 09a6ad7 into master Apr 30, 2021
@alexanderbez alexanderbez deleted the bez/6403-change-event-types branch April 30, 2021 13:27
tychoish pushed a commit to tychoish/tendermint that referenced this pull request May 5, 2021
@alexanderbez @tychoish
types: Refactor EventAttribute (tendermint#6408)
b912266
@julienrbrt julienrbrt mentioned this pull request May 23, 2022
Closed
4 tasks
@Jiuhk Jiuhk mentioned this pull request Aug 25, 2022
Closed
@thanethomson thanethomson mentioned this pull request Aug 25, 2022
Closed
thanethomson pushed a commit that referenced this pull request Aug 30, 2022
@alexanderbez @thanethomson
types: Refactor EventAttribute (#6408)
1753fff 
Cherry-pick of 09a6ad7
@thanethomson thanethomson mentioned this pull request Aug 30, 2022
Merged
3 tasks
thanethomson added a commit that referenced this pull request Aug 30, 2022
@thanethomson @alexanderbez
abci: Port EventAttribute field type change to main (#9335)
7b40167 
* types: Refactor EventAttribute (#6408)

Cherry-pick of 09a6ad7

* Ensure context is honored

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Replace with tagged switch

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Ensure contexts are honored

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Add UPGRADING note about type change

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Remove unnecessary conversion

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Signed-off-by: Thane Thomson <connect@thanethomson.com>
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
@yito88 yito88 mentioned this pull request Sep 21, 2022
Closed
@LCyson LCyson mentioned this pull request Mar 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ethanfrey ethanfrey ethanfrey left review comments

@tac0turtle tac0turtle tac0turtle approved these changes

@cmwaters cmwaters Awaiting requested review from cmwaters

@ebuchman ebuchman Awaiting requested review from ebuchman

@tessr tessr Awaiting requested review from tessr

@tychoish tychoish Awaiting requested review from tychoish

Assignees
No one assigned
Labels
C:abci Component: Application Blockchain Interface C:events Component: Events S:automerge Automatically merge PR when requirements pass
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Events get mangled when non-utf8 value in Value
3 participants
@alexanderbez @ethanfrey @tac0turtle