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

Amm info handler new #857

Closed

Conversation

arihantkothari
Copy link
Collaborator

@arihantkothari arihantkothari commented Sep 18, 2023

  • Adds amm_info handler to clio

@arihantkothari arihantkothari marked this pull request as draft September 18, 2023 16:27
@godexsoft godexsoft added this to the 2.1 milestone Sep 19, 2023
@godexsoft godexsoft added the enhancement New feature or request label Sep 19, 2023
@arihantkothari
Copy link
Collaborator Author

The amm_info can be fetch by "amm_account" field. But I did not see the implementation of it. Is it in the PR's scope? https://xrpl.org/amm_info.html#amm_info Since it is already in the document, I think we need to implement it .

I implemented this in my last commit, thanks for looking into it carefully.

@arihantkothari arihantkothari marked this pull request as ready for review November 20, 2023 16:47
{
using namespace ripple;

if (!input.ammAccount && (input.issue1 == ripple::noIssue() || input.issue2 == ripple::noIssue()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

we need to check :
1 ammAccount and issue1/2 can not be set at the same time
2 ammAccount or issue1/2 must be set


auto const amm = SLE{SerialIter{ammBlob->data(), ammBlob->size()}, ammKeylet.key};
auto const ammAccountID = amm.getAccountID(sfAccount);
auto const accBlob =
Copy link
Collaborator

@cindyyan317 cindyyan317 Nov 21, 2023

Choose a reason for hiding this comment

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

do we need to check the existence of ammAccountID which is from the amm ledger object 🤔

static auto const rpcSpec = RpcSpec{
{JS(ledger_hash), validation::Uint256HexStringValidator},
{JS(ledger_index), validation::LedgerIndexValidator},
{JS(asset), validation::ammAssetValidator},
Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand the ammAssetValidator is from ledger_entry . You may need to bespoke the error code 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.

I removed the ammAssetValidator from ledger_entry and added it to validators. What do you mean by "bespoke the error code" ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Try to input invalid asset , you will see rippled raises different error for the ledger_entry and amm_info.

@arihantkothari
Copy link
Collaborator Author

Some example payloads that might be useful:

curl -d '{
    "method": "amm_info",
    "params": [{
    "amm_account":"rLcS7XL6nxRAi7JcbJcn1Na179oF3vdfbh"
    }]
}' http://localhost:51233

curl -d '{
    "method": "amm_info",
    "params": [{
      "asset": {
        "currency": "CNY", "issuer":"rBdLS7RVLqkPwnWQCT2bC6HJd6xGoBizq8"
      },
      "asset2": {
        "currency": "USD",
        "issuer": "rBdLS7RVLqkPwnWQCT2bC6HJd6xGoBizq8"
      }
    }]
}' http://localhost:51233

curl -d '{
    "method": "amm_info",
    "params": [{
    "amm_account":"rspfN8ESWkkDLxBD9MT6fvxbytnCijnYh9"
    }]
}' http://localhost:51233

@arihantkothari
Copy link
Collaborator Author

I have pushed the amm_info unit tests but they are really incomplete. This is the only thing that is incomplete in the PR.

@godexsoft
Copy link
Collaborator

Closing this one in favour of #1060

@godexsoft godexsoft closed this Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants