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

Change order of checks in amm_info #4924

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

Bronek
Copy link
Collaborator

@Bronek Bronek commented Feb 20, 2024

High Level Overview of Change

Checking if account is well-formed currently happens before the check if asset , asset2 and amm_account are supplied, which results in an unexpected error messages if any of the fields is malformed. Swap the order of checks, so validity of fields is verified first. Since this is a breaking change, retain the existing behaviour if (apiVersion < 3) and add identical check, but after checks of assets and accounts, if (apiVersion >= 3). Also add unit tests to demonstrate the old and new behaviour.

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)
  • Performance (increase or change in throughput and/or latency)
  • Tests (you added tests for code that already exists, or your new feature included in this PR)
  • Documentation update
  • Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
  • Release

API Impact

  • Public API: New feature (new methods and/or new fields)
  • Public API: Breaking change (in general, breaking changes should only impact the next api_version)
  • libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
  • Peer protocol change (must be backward compatible or bump the peer protocol version)

Before: sending an invalid account or asset to amm_info RPC method while other parameters are not set as expected would result in rpcINVALID_PARAMS error. This behaviour is retained in API version 2

After: sending an invalid account or asset to amm_info RPC method while other parameters are not set as expected will result in rpcISSUE_MALFORMED or rpcACT_MALFORMED error. This behaviour is new in API version 3

This resolves XRPLF/clio#1157

@Bronek Bronek marked this pull request as ready for review February 21, 2024 12:55
@Bronek Bronek force-pushed the feature/change_amm_info_error branch from 58ef31b to 99ea4fe Compare February 21, 2024 13:03
@codecov-commenter
Copy link

codecov-commenter commented Feb 21, 2024

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 61.50%. Comparing base (f650949) to head (0ac0c1b).
Report is 1 commits behind head on develop.

❗ Current head 0ac0c1b differs from pull request most recent head 22f455f. Consider uploading reports for the commit 22f455f to get more accurate results

Files Patch % Lines
src/ripple/rpc/handlers/AMMInfo.cpp 77.77% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4924      +/-   ##
===========================================
- Coverage    70.93%   61.50%   -9.44%     
===========================================
  Files          796      797       +1     
  Lines        66792    70163    +3371     
  Branches     10998    36264   +25266     
===========================================
- Hits         47377    43151    -4226     
- Misses       19415    19764     +349     
- Partials         0     7248    +7248     

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

Copy link
Collaborator

@godexsoft godexsoft left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@intelliot intelliot added Clio Reviewed APIv3 Change does not take effect unless client opts-in to api_version 3 (currently beta) labels Feb 22, 2024
src/test/rpc/AMMInfo_test.cpp Outdated Show resolved Hide resolved
src/test/rpc/AMMInfo_test.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@gregtatcam gregtatcam left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@Bronek
Copy link
Collaborator Author

Bronek commented Feb 27, 2024

this is good to merge - note it preserves the current behaviour in API version 2 and adds a test for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APIv3 Change does not take effect unless client opts-in to api_version 3 (currently beta) Clio Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[amm_info] with malformed account error differs with rippled
6 participants