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(blob)!: tendermint compatible commitment proof json marshall #3929

Open
wants to merge 8 commits into
base: feature/api-breaks
Choose a base branch
from

Conversation

zvolin
Copy link

@zvolin zvolin commented Nov 11, 2024

Fixes #3918

Please note: this PR is BREAKING as commitment proof encoding has changed.

@github-actions github-actions bot added the external Issues created by non node team members label Nov 11, 2024
@zvolin zvolin force-pushed the fix/tendermint-compatible-commitment-proof branch from 9b64710 to 89178ee Compare November 11, 2024 14:08
Copy link
Member

@rach-id rach-id left a 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?

@zvolin
Copy link
Author

zvolin commented Nov 11, 2024

yes, it uses coretypes.RowProof. Everything that comes from core (tendermint fork) should be serialized with tendermint json encoder / decoder as it treats 64 bit integers differently (stringifies them)

@rach-id
Copy link
Member

rach-id commented Nov 11, 2024

thanks for your response @zvolin. And why is this change useful?

@rach-id
Copy link
Member

rach-id commented Nov 11, 2024

After second thinking about this, I guess you're right, this needs fixing

@zvolin
Copy link
Author

zvolin commented Nov 11, 2024

And why is this change useful?

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 zvolin changed the base branch from main to feature/api-breaks November 11, 2024 16:34
@cristaloleg cristaloleg added kind:break! Attached to breaking PRs kind:fix Attached to bug-fixing PRs labels Nov 12, 2024
@vgonkivs
Copy link
Member

@zvolin could you please fix lint?

@zvolin zvolin force-pushed the fix/tendermint-compatible-commitment-proof branch from 8422d77 to 90741ea Compare November 13, 2024 11:59
@zvolin
Copy link
Author

zvolin commented Nov 13, 2024

switched to marshaling whole type as per #3930 (comment)

@vgonkivs
Copy link
Member

Linter failed.

@vgonkivs vgonkivs self-requested a review November 13, 2024 14:14
@zvolin
Copy link
Author

zvolin commented Nov 14, 2024

should be good now, sorry

Copy link
Member

@renaynay renaynay left a 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

@vgonkivs
Copy link
Member

blob_fuzz_test.go:69: invalid 64-bit integer encoding "64", expected string

@zvolin
Copy link
Author

zvolin commented Nov 15, 2024

🤞 sd '"(total|index)":([0-9]+)' '"$1":"$2"' blob/testdata/**/*.json

@codecov-commenter
Copy link

codecov-commenter commented Nov 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (feature/api-breaks@4a90605). Learn more about missing BASE report.

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.
📢 Have feedback on the report? Share it here.

Copy link
Member

@vgonkivs vgonkivs left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 . Thanks @zvolin

@zvolin
Copy link
Author

zvolin commented Nov 15, 2024

make test-integration SHORT=true TAGS=sync passes for me locally 🤔

@vgonkivs
Copy link
Member

The test seems to be flaky.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external Issues created by non node team members kind:break! Attached to breaking PRs kind:fix Attached to bug-fixing PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Serialization inconsistency between celestia-node API and Tendermint API
6 participants