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.

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

fix: modify proto pubkey regisiter name for IBC relayer compatibility #762

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

tkxkd0159
Copy link
Member

@tkxkd0159 tkxkd0159 commented Feb 29, 2024

Description

  1. IBC relayer(hermes) parse events based on tendermint-rs. It renames fields as tendermint.crypto.* via serde for PublicKey. (ref)
  2. To fix above issue, we should modify register name for our proto pubkey. This is only used for converting to JSON. And Ostracon doesn't save these values anywhere so there are no breaking changes even if we change this. (saved consensus key with internal type, not proto type)
  3. There is no breaking change for finschia-sdk. sdk registers their amino types here.

Closes: #XXX

@tkxkd0159 tkxkd0159 self-assigned this Feb 29, 2024
@CLAassistant
Copy link

CLAassistant commented Feb 29, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

codecov bot commented Mar 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.64%. Comparing base (2aa7d4c) to head (489777b).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #762      +/-   ##
==========================================
+ Coverage   66.54%   66.64%   +0.10%     
==========================================
  Files         285      285              
  Lines       37919    37919              
==========================================
+ Hits        25232    25271      +39     
+ Misses      10883    10850      -33     
+ Partials     1804     1798       -6     
Files Coverage Δ
crypto/encoding/codec.go 47.61% <100.00%> (ø)

... and 14 files with indirect coverage changes

@tkxkd0159 tkxkd0159 marked this pull request as ready for review March 4, 2024 02:45
@ulbqb
Copy link
Member

ulbqb commented Mar 4, 2024

https://github.com/tendermint/tendermint/blob/main/crypto/encoding/codec.go
Is it possible to use tendermint package in your situation? It looks like codec file should be removed in Ostracon because of duplication.

@tkxkd0159
Copy link
Member Author

https://github.com/tendermint/tendermint/blob/main/crypto/encoding/codec.go Is it possible to use tendermint package in your situation? It looks like codec file should be removed in Ostracon because of duplication.

Yeah I think it's possible. But I have heard that our repo will be converted with cometbft soon. So I just tried to not change as much as possible. Is it better to replace with tendermint now? @ulbqb

@ulbqb
Copy link
Member

ulbqb commented Mar 4, 2024

So I just tried to not change as much as possible.

That’s make sense.

@tkxkd0159 tkxkd0159 merged commit fc54d22 into main Mar 4, 2024
37 checks passed
@tkxkd0159 tkxkd0159 deleted the fix-pubkey-incompatible branch March 4, 2024 06:58
@ulbqb ulbqb added the C: enhancement Classification: New feature or its request, or improvement in maintainability of code label Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: enhancement Classification: New feature or its request, or improvement in maintainability of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants