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 checking ammID field of account #5188

Merged
merged 3 commits into from
Nov 18, 2024
Merged

Conversation

oleks-rip
Copy link
Collaborator

High Level Overview of Change

This fix add check for sfAMMID field in amm_info handler.

Context of Change

Without this check debug version of rippled crash on assert in ledger::read()
Bug was found during automation testing.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

API Impact

  • Public API: amm_info will not crash debug version of the rippled

Copy link

codecov bot commented Nov 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.9%. Comparing base (838978b) to head (a9f6586).
Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #5188     +/-   ##
=========================================
- Coverage     77.9%   77.9%   -0.0%     
=========================================
  Files          782     782             
  Lines        66621   66622      +1     
  Branches      8161    8158      -3     
=========================================
- Hits         51902   51874     -28     
- Misses       14719   14748     +29     
Files with missing lines Coverage Δ
src/xrpld/rpc/handlers/AMMInfo.cpp 92.6% <100.0%> (+0.1%) ⬆️

... and 8 files with indirect coverage changes

Impacted file tree graph

---- 🚨 Try these New Features:

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

@kennyzlei kennyzlei added this to the 2.3.0 (Nov 2024) milestone Nov 13, 2024
@shawnxie999
Copy link
Collaborator

Unrelated to this change, but on line 162, an non existent AMM object is returned with rpcACT_NOT_FOUND error. Doesn't sound right, should we fix it?

        auto const amm = ledger->read(ammKeylet);
        if (!amm)
            return Unexpected(rpcACT_NOT_FOUND);

Comment on lines +322 to +334
using namespace jtx;
testcase("Invalid amm field");

testAMM([&](AMM& amm, Env&) {
auto const resp = amm.ammRpcInfo(
std::nullopt,
jss::validated.c_str(),
std::nullopt,
std::nullopt,
gw);
BEAST_EXPECT(
resp.isMember("error") && resp["error"] == "actNotFound");
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: the test could be moved into testErrors() IMO

@gregtatcam
Copy link
Collaborator

Unrelated to this change, but on line 162, an non existent AMM object is returned with rpcACT_NOT_FOUND error. Doesn't sound right, should we fix it?

        auto const amm = ledger->read(ammKeylet);
        if (!amm)
            return Unexpected(rpcACT_NOT_FOUND);

It's half correct since AMM might be fetched by amm account. But correct, rpcOBJECT_NOT_FOUND is probably more appropriate.

@shawnxie999
Copy link
Collaborator

Unrelated to this change, but on line 162, an non existent AMM object is returned with rpcACT_NOT_FOUND error. Doesn't sound right, should we fix it?

        auto const amm = ledger->read(ammKeylet);
        if (!amm)
            return Unexpected(rpcACT_NOT_FOUND);

It's half correct since AMM might be fetched by amm account. But correct, rpcOBJECT_NOT_FOUND is probably more appropriate.

although i believe we may need a new API version for this change

@oleks-rip
Copy link
Collaborator Author

Ready to be merged.

@ximinez ximinez merged commit 0ec17b6 into XRPLF:develop Nov 18, 2024
7 of 8 checks passed
@oleks-rip oleks-rip deleted the amm_info_fix branch November 18, 2024 19:02
@ximinez ximinez mentioned this pull request Nov 18, 2024
1 task
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.

7 participants