-
Notifications
You must be signed in to change notification settings - Fork 512
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
feat: add sidechain-specific RPCs/sugar #1940
Conversation
* | ||
* @category Responses | ||
*/ | ||
export interface FederatorInfoResponse extends BaseResponse { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these values have docstrings/comments explaining what they are used for like we do with other requests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - however, those don't exist yet.
* @throws XrplError - if there are more than 2 memos. | ||
* @category Utilities | ||
*/ | ||
export default function createXchainPayment( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming wise I think it's better to explicitly write "Cross" rather than "X" since X is actually meaningful sometimes in our libraries (Ex. xAddress)
Also, "chain" should be capitalized.
I'd recommend to rename this function to createCrossChainPayment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Let's use "Cross" instead to avoid ambiguity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just need to change the "X" wording to "Cross". Otherwise, LGTM!
* @throws XrplError - if there are more than 2 memos. | ||
* @category Utilities | ||
*/ | ||
export default function createXchainPayment( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Let's use "Cross" instead to avoid ambiguity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
High Level Overview of Change
This PR:
federator_info
request and response objectsContext of Change
xrpl.js sidechain support
Type of Change
Before / After
Test Plan
Added tests for the helper method. I'm not sure how to test the RPC call and object, since standalone rippled isn't a federator. CI passes.