Skip to content

types: Refactor EventAttribute#6408

Merged
alexanderbez merged 9 commits intomasterfrom
bez/6403-change-event-types
Apr 30, 2021
Merged

types: Refactor EventAttribute#6408
alexanderbez merged 9 commits intomasterfrom
bez/6403-change-event-types

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

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

thanethomson added a commit that referenced this pull request Aug 30, 2022
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C:abci Component: Application Blockchain Interface C:events Component: Events S:automerge Automatically merge PR when requirements pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Events get mangled when non-utf8 value in Value

3 participants