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

ledger: Implement JSON encoding and decoding for ledgercore.StateDelta #6100

Closed
wants to merge 5 commits into from

Conversation

Ashy5000
Copy link
Contributor

Summary

Implements MarshalJSON() and UnmarshalJSON() methods on the ledgercore.StateDelta struct that can properly encode maps with objects as keys.

Test Plan

To test JSON encoding/decoding on ledgercore.StateDelta, I've added a unit test that uses the json.Marshal() to encode the StateDelta, then json.Unmarshal() to decode it. If the initial StateDelta value matches the decoded value, the test passes. Otherwise, it fails. All tests are in ledger/ledgercore/statedelta_test.go.

@CLAassistant
Copy link

CLAassistant commented Aug 11, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

codecov bot commented Aug 11, 2024

Codecov Report

Attention: Patch coverage is 81.81818% with 12 lines in your changes missing coverage. Please review.

Project coverage is 55.89%. Comparing base (23a04c2) to head (1ebc7e3).
Report is 9 commits behind head on master.

Files with missing lines Patch % Lines
ledger/ledgercore/statedelta.go 81.81% 6 Missing and 6 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6100      +/-   ##
==========================================
- Coverage   56.25%   55.89%   -0.36%     
==========================================
  Files         490      490              
  Lines       69699    69765      +66     
==========================================
- Hits        39206    38995     -211     
- Misses      27825    28061     +236     
- Partials     2668     2709      +41     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@algorandskiy
Copy link
Contributor

IMO if one needs StateDelta as JSON they need to reencode it from msgpack outside of algod. I do not think there is a place for a StateDelta copy in ledgercore.
Background: ledgercore.StateDelta was thought as internal to go-algorand packages but at some point got exposed via REST API (that I think it should not have been). No JSON representation is needed for its intended use.

I let others to comment before closing this.

@jasonpaulos
Copy link
Contributor

@algorandskiy I agree it may not make sense to expose this as JSON. The only real value I can think of is as a debugging tool, but as you pointed out, converting the response from msgpack to JSON can achieve that as well.

I think there are two choices to fix the problem here:

  1. Fix the JSON encoding, like this PR does
  2. Or remove the ability to ask for a JSON-formatted state delta from the REST API

While 2 is technically a breaking change, it is simpler and there is likely little to no JSON usage of the affected endpoints.

@Ashy5000
Copy link
Contributor Author

The only problem with converting msgpack to JSON is that JSON doesn't support using objects as keys, requiring extra conversion steps. I think it's a lot easier if JSON can be requested directly, with no conversion needed.

@jasonpaulos
Copy link
Contributor

Hi @Ashy5000, I've opened #6106 as a continuation of this PR. I've kept the spirit of your original proposal, but I ended up changing a lot of the implementation details. Please let me know what you think of that.

@algorandskiy
Copy link
Contributor

Closing as #6106 was merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants