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

APIv2(gateway_balances, channel_authorize): update errors #4618

Merged
merged 32 commits into from Sep 21, 2023

Conversation

PeterChen13579
Copy link
Contributor

High Level Overview of Change

Fixed/added error messages for Gateway_balance(issue #4290 | #4548 ) and channel_authorize(#4289 )

Context of Change

Added error messages when user sends an incorrect request. (Check the above Github issue link for more info.) Everything is under API Version 2 as it is a breaking change.

Type of Change

  • [ X] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ X] Tests (You added tests for code that already exists, or your new feature included in this PR)

Added own unit test for API Version 2;

@PeterChen13579
Copy link
Contributor Author

@gregtatcam @shawnxie999 Hi guys, I accidentally messed up my git, so I had to create a new PR. This one is exactly the same changes as my old one(#4577); please approve this PR if everything looks good. Thank you

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.

👍

@intelliot
Copy link
Collaborator

@PeterChen13579 this PR currently has merge conflicts which you'll need to resolve (preferably with a merge commit).

@gregtatcam do you have an opinion on whether it's OK for this PR to be merged before #4626?

(I have no preference)

@gregtatcam
Copy link
Collaborator

@PeterChen13579 this PR currently has merge conflicts which you'll need to resolve (preferably with a merge commit).

@gregtatcam do you have an opinion on whether it's OK for this PR to be merged before #4626?

(I have no preference)

I have no preference either.

@intelliot
Copy link
Collaborator

@PeterChen13579 - please resolve conflicts - preferably by pushing a merge commit (do not force-push).

@intelliot
Copy link
Collaborator

Suggested commit message:

APIv2(gateway_balances, channel_authorize): update errors (#4618)

Since these are breaking changes, they apply only to API version 2.
* Supersedes #4577

gateway_balances
* When `account` does not exist in the ledger, return `actNotFound`
  * (Previously, a normal response was returned)
  * Fix #4290
* When required field(s) are missing, return `invalidParams`
  * (Previously, `invalidHotWallet` was incorrectly returned)
  * Fix #4548

channel_authorize
* When the specified `key_type` is invalid, return `badKeyType`
  * (Previously, `invalidParams` was returned)
  * Fix #4289

@connorjchen
Copy link

LGTM

@intelliot intelliot changed the title Added Error messages on Gateway_Balance and Channel_Authorize #4577 APIv2(gateway_balances, channel_authorize): update errors Jul 18, 2023
@intelliot intelliot added this to the 1.13 milestone Aug 23, 2023
@intelliot
Copy link
Collaborator

note: conflict is expected to go away once #4504 is merged

@intelliot
Copy link
Collaborator

  • note: rippled catching up with what Clio already does

@intelliot intelliot merged commit 2487dab into XRPLF:develop Sep 21, 2023
15 checks passed
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 22, 2023
gateway_balances
* When `account` does not exist in the ledger, return `actNotFound`
  * (Previously, a normal response was returned)
  * Fix XRPLF#4290
* When required field(s) are missing, return `invalidParams`
  * (Previously, `invalidHotWallet` was incorrectly returned)
  * Fix XRPLF#4548

channel_authorize
* When the specified `key_type` is invalid, return `badKeyType`
  * (Previously, `invalidParams` was returned)
  * Fix XRPLF#4289

Since these are breaking changes, they apply only to API version 2.

Supersedes XRPLF#4577
@intelliot intelliot mentioned this pull request Sep 22, 2023
1 task
intelliot pushed a commit that referenced this pull request Sep 27, 2023
Fix the Windows build by using `unsigned int` (instead of `uint`).

The error, introduced by #4618, looks something like:
  rpc\impl\RPCHelpers.h(299,5): error C2061: syntax error: identifier
  'uint' (compiling source file app\ledger\Ledger.cpp)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment