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

from_xrpl should not accept snake_case dict keys #694

Merged
merged 18 commits into from
Jun 21, 2024
Merged

from_xrpl should not accept snake_case dict keys #694

merged 18 commits into from
Jun 21, 2024

Conversation

ckeshava
Copy link
Collaborator

@ckeshava ckeshava commented Mar 6, 2024

BaseModel.from_xrpl method should not accept snake_case keys in dictionary input.

Fix #689 . You can find more context in the discussion pertaining to this issue.

I have used a regular expression matches for snake_case. If you have any other ideas for detecting non-camelCase input formats, let me know. There appears to be a variety of solutions for this problem, but all of them have slight differences in their behavior.

High Level Overview of Change

Context of Change

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

Did you update CHANGELOG.md?

  • Yes
  • No, this change does not impact library users
    Do external users use from_xrpl method? I believe they use from_dict (hasn't been modified) which internally makes a call to from_xrpl

Test Plan

xrpl/models/base_model.py Outdated Show resolved Hide resolved
xrpl/models/base_model.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@mvadari mvadari left a comment

Choose a reason for hiding this comment

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

Please update the changelog

@ckeshava ckeshava requested a review from mvadari March 6, 2024 23:31
@ckeshava
Copy link
Collaborator Author

ckeshava commented Mar 8, 2024

The integration and snippet tests are not reliable. I'm observing these errors: (Integration Test Python 3.8): xrpl.asyncio.wallet.wallet_generation.XRPLFaucetException: Unable to fund address with faucet after waiting 40 seconds

Snippet test error: httpx.HTTPStatusError: Server error '502 Bad Gateway' for url 'https://faucet.altnet.rippletest.net/accounts'

xrpl/models/base_model.py Outdated Show resolved Hide resolved
xrpl/models/base_model.py Outdated Show resolved Hide resolved
xrpl/models/base_model.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
xrpl/models/base_model.py Outdated Show resolved Hide resolved
@ckeshava
Copy link
Collaborator Author

@justinr1234 @khancode can you please review this at your convinience?

Copy link
Collaborator

@khancode khancode left a comment

Choose a reason for hiding this comment

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

LGTM

@ckeshava ckeshava requested a review from mvadari April 8, 2024 20:07
# Note: BaseModel.from_xrpl and its overridden methods accept only camelCase or
# PascalCase inputs (i.e. snake_case is not accepted)
def test_from_xrpl_accepts_only_camel_case_inputs(self):
request = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the difference between this test case and tx_snake_case_keys? Just want to make sure I'm following what's going on here :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hello, both the tests dis-allow snake_case keys in Request and Transaction models, respectively. I used different inputs for two different models

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you split it into 2 tests (2 functions)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed in 58ff257

@ckeshava ckeshava requested a review from pdp2121 May 7, 2024 17:34
@ckeshava
Copy link
Collaborator Author

I have updated the regular-expression which is used to invalidate inputs in BaseModel. This was required after the introduction of the AMM feature. (AMM introduced a new type of key Amount2, with an integer in its wake -- this was breaking the regexp)

If anyone has objections pertaining to it, please let me know. It is not related to the changelist of this PR.

@ckeshava ckeshava merged commit 173672e into main Jun 21, 2024
21 checks passed
@ckeshava ckeshava deleted the noCamelCase branch June 21, 2024 21:37
ckeshava added a commit to ckeshava/xrpl-py that referenced this pull request Jul 1, 2024
* BaseModel.from_xrpl method should not accept snake_case keys in dictionary input

* Fix integration test: Update regex to accomodate Amount2 AMM field

---------

Co-authored-by: Mayukha Vadari <mvadari@ripple.com>
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.

Fix LedgerObject.from_xrpl to only accept CamelCase JSON keys
5 participants