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: skip serialization of continuation token if None #530

Merged

Conversation

apoorvsadana
Copy link
Contributor

@apoorvsadana apoorvsadana commented Dec 18, 2023

In the Starknet Spec, the continuation_token is not a required field. So in the last event page, the continuation_token must be absent from the response instead of being null. I also tried this with the infura RPC (which uses pathfinder?) and works in the same way. We are integrating Madara with the explorer and that's breaking because of this.

Copy link
Contributor

@glihm glihm left a comment

Choose a reason for hiding this comment

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

Looks good, I would only add in the comment the reason why it's added. 👍

@apoorvsadana
Copy link
Contributor Author

Done!

@xJonathanLEI
Copy link
Owner

Does this need to be backported or are you guys already using the latest commit?

@apoorvsadana
Copy link
Contributor Author

We're already using a specific rev. We can change that to the latest one, shouldn't be an issue.

@xJonathanLEI
Copy link
Owner

Are you using an old rev with JSON-RPC pre-0.6.0 though? The latest commit uses pre-0.6.0.

@xJonathanLEI
Copy link
Owner

Or are you only using this one single type? Then it shouldn't matter. Not sure how the lib is used in Madara.

Copy link
Owner

@xJonathanLEI xJonathanLEI 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 for spotting. Really all Option fields should be skipped for serialization when it's None.

@xJonathanLEI xJonathanLEI changed the title skip serialization of continuation token if None fix: skip serialization of continuation token if None Dec 21, 2023
@xJonathanLEI xJonathanLEI merged commit 6cadb19 into xJonathanLEI:master Dec 21, 2023
24 checks passed
@apoorvsadana
Copy link
Contributor Author

We are using it for other things as well and currently Madara isn't on 0.6.0. So you're right, just updating the commit might break. We are on this rev right now - a35ce22. Is it possible to backport it?

@xJonathanLEI
Copy link
Owner

Oh that's a very old commit, not even JSON-RPC v0.5.1.. still on v0.4.0.

@xJonathanLEI
Copy link
Owner

In this case maybe you can backport this in a fork and use the fork for Madara until it upgrades?

@apoorvsadana
Copy link
Contributor Author

Ya sure we can do that then. Thanks for the help!

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.

3 participants