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 native event metadata parsing #197

Merged
merged 11 commits into from
Jun 14, 2024
Merged

Conversation

ziscky
Copy link
Contributor

@ziscky ziscky commented Jun 12, 2024

https://app.clickup.com/t/8694tyanq

This PR addresses the issue of misrepresenting event values containing CIDs and BigInts in the parsed metadata.

  1. CIDs were wrongly represented as {}.
  2. BigInts were wrongly represented as the base64 encoding of the raw bytes.

Tests have also been updated accordingly.

References:

🔗 zboto Link

Copy link
Member

@emmanuelm41 emmanuelm41 left a comment

Choose a reason for hiding this comment

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

🚀🚀

tools/events/decode.go Outdated Show resolved Hide resolved
tools/events/decode.go Outdated Show resolved Hide resolved
@ziscky ziscky requested a review from emmanuelm41 June 13, 2024 16:27
@ziscky ziscky marked this pull request as ready for review June 13, 2024 16:31
Copy link
Member

@emmanuelm41 emmanuelm41 left a comment

Choose a reason for hiding this comment

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

Amazing! Really amazing!

tools/events/decode.go Outdated Show resolved Hide resolved
@emmanuelm41
Copy link
Member

@ziscky can we update the PR description?

@ziscky ziscky requested a review from emmanuelm41 June 14, 2024 08:49
Copy link
Member

@emmanuelm41 emmanuelm41 left a comment

Choose a reason for hiding this comment

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

Let's fix tests first! Besides, please check int64 does not fit on JSON format. Those should be strings too :/ Check the max JSON number! Made me think we could represent numbers as strings directly. Not sure what it is easier to do.

@ziscky ziscky requested a review from emmanuelm41 June 14, 2024 14:50
Copy link
Member

@emmanuelm41 emmanuelm41 left a comment

Choose a reason for hiding this comment

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

Just wondering if the value can be negative, as the ipld kind is Int, and we later compare to Uint32... My only doubt here!

@ziscky
Copy link
Contributor Author

ziscky commented Jun 14, 2024

Just wondering if the value can be negative, as the ipld kind is Int, and we later compare to Uint32... My only doubt here!

Updated to check both bounds

@emmanuelm41 emmanuelm41 merged commit cdf991c into dev Jun 14, 2024
5 checks passed
@emmanuelm41 emmanuelm41 deleted the fix/native-event-metadata branch June 14, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants