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

refactor: redo binary codec types to simplify #396

Merged
merged 5 commits into from
Jun 13, 2022
Merged

Conversation

mvadari
Copy link
Collaborator

@mvadari mvadari commented Jun 2, 2022

High Level Overview of Change

This PR:

  • Fixes a typing issue with the binary parser that was caused by a circular dependency (which we now know how to resolve)
  • Renames SerializedDict -> STObject and SerializedList -> STArray (to match rippled names)
  • Refactors the list of types in FieldInstance to pull directly from xrpl.core.binarycodec.types, so that doesn't need to be edited every time a new type is added

There are no feature changes here; this is just an internal refactor.

Context of Change

I was adding some new types to the binary codec and thought that needing to add them in 2 places was unnecessary. I fixed a few other things to make the binary codecs more analogous as well.

Type of Change

  • Refactor (non-breaking change that only restructures code)

Test Plan

CI passes. Tests that test the internal objects were edited to use the new type names (the methods exposed externally have not changed at all).

Copy link
Contributor

@JST5000 JST5000 left a comment

Choose a reason for hiding this comment

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

LGTM

@ckniffen
Copy link
Collaborator

@mvadari Would you consider minor version bump?

@mvadari
Copy link
Collaborator Author

mvadari commented Jun 13, 2022

@mvadari Would you consider minor version bump?

There's literally no feature change - this PR is just refactors. No one should be using this library at the depth where I changed things.

We'll have to do a minor version bump for #397 anyways, so it can be rolled into that.

@mvadari mvadari merged commit 5d51072 into master Jun 13, 2022
@mvadari mvadari deleted the fix-binary-codec-types branch June 13, 2022 21:23
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