-
Notifications
You must be signed in to change notification settings - Fork 925
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(blob)!: tendermint compatible commitment proof json marshall #3929
base: feature/api-breaks
Are you sure you want to change the base?
fix(blob)!: tendermint compatible commitment proof json marshall #3929
Conversation
9b64710
to
89178ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening this PR.
Do you think we will still need the JSON marshall/unmarshall after merging this #3821 to main?
yes, it uses |
thanks for your response @zvolin. And why is this change useful? |
After second thinking about this, I guess you're right, this needs fixing |
to have unified representation of objects regardless whether they are returned from celestia-node api or tendermint rest api. Moreover, tendermint implementations in different languages can have the rules how to encode types in different level than in go, namely tendermint-rs bakes how the type is meant to be encoded on type level, not encoder level, so in lumina we had to patch tendermint to achieve celestia-node compatible representation, but we could never be compatible with both tendermint and celestia-node. tl;dr it originates from us wishing to drop tendermint-rs fork but I believe it's a good thing in general |
@zvolin could you please fix lint? |
8422d77
to
90741ea
Compare
switched to marshaling whole type as per #3930 (comment) |
Linter failed. |
should be good now, sorry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zvolin some broken tests
|
🤞 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/api-breaks #3929 +/- ##
=====================================================
Coverage ? 45.40%
=====================================================
Files ? 307
Lines ? 21812
Branches ? 0
=====================================================
Hits ? 9904
Misses ? 10846
Partials ? 1062 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀 . Thanks @zvolin
|
The test seems to be flaky. |
Fixes #3918
Please note: this PR is BREAKING as commitment proof encoding has changed.