update Paths snippet test #2765
Conversation
… test, remove dependence on theexternal state of testnet
|
This was the easiest design that I could find, to demonstrate the use of |
|
@CodeRabbit full review |
|
@coderabbitai full review |
Actions performedFull review triggered. |
|
Warning Rate limit exceeded@ckeshava has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 22 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in the Changes
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
packages/xrpl/snippets/src/paths.ts
Outdated
| await client.submitAndWait( | ||
| { | ||
| TransactionType: 'AccountSet', | ||
| Account: alice.classicAddress, | ||
| SetFlag: AccountSetAsfFlags.asfDefaultRipple, | ||
| }, | ||
| { | ||
| wallet: alice, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Refactor repeated AccountSet transactions into a helper function
The code for setting the asfDefaultRipple flag is repeated for Alice, Bob, and Carol. Refactoring this into a helper function enhances maintainability and adheres to the DRY (Don't Repeat Yourself) principle.
Here's a suggested helper function:
async function setDefaultRipple(client: Client, wallet: Wallet): Promise<void> {
await client.submitAndWait(
{
TransactionType: 'AccountSet',
Account: wallet.classicAddress,
SetFlag: AccountSetAsfFlags.asfDefaultRipple,
},
{ wallet },
)
}You can then use this function for each wallet:
await setDefaultRipple(client, alice)
await setDefaultRipple(client, bob)
await setDefaultRipple(client, carol)Also applies to: 32-41, 43-52
packages/xrpl/snippets/src/paths.ts
Outdated
| await client.submitAndWait( | ||
| { | ||
| TransactionType: 'TrustSet', | ||
| Account: bob.classicAddress, | ||
| LimitAmount: { | ||
| currency: 'USD', | ||
| issuer: alice.classicAddress, | ||
| value: '1000', | ||
| }, | ||
| }, | ||
| { | ||
| wallet: bob, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Refactor repeated TrustSet transactions into a helper function
The code for establishing trust lines from Bob to Alice and Carol to Bob is similar. Creating a helper function reduces duplication and improves code readability.
Suggested helper function:
async function setTrustLine(
client: Client,
wallet: Wallet,
currency: string,
issuer: string,
): Promise<void> {
await client.submitAndWait(
{
TransactionType: 'TrustSet',
Account: wallet.classicAddress,
LimitAmount: {
currency,
issuer,
value: '1000',
},
},
{ wallet },
)
}Usage:
await setTrustLine(client, bob, 'USD', alice.classicAddress)
await setTrustLine(client, carol, 'USD', bob.classicAddress)Also applies to: 70-83
packages/xrpl/snippets/src/paths.ts
Outdated
| const path = paths[0].paths_computed[0][0] | ||
| if (path.account !== bob.classicAddress) { | ||
| throw new Error('Path does not include bob') | ||
| } |
There was a problem hiding this comment.
Add checks to prevent possible runtime errors when accessing nested paths
Accessing paths[0].paths_computed[0][0] may throw an error if any of the arrays are empty. Adding checks ensures the arrays contain the expected elements.
Proposed modification:
const paths = response.result.alternatives
if (paths.length === 0) {
throw new Error('No paths found')
}
+if (
+ !paths[0].paths_computed ||
+ paths[0].paths_computed.length === 0 ||
+ paths[0].paths_computed[0].length === 0
+) {
+ throw new Error('No computed paths available')
+}
const path = paths[0].paths_computed[0][0]
if (path.account !== bob.classicAddress) {
throw new Error('Path does not include Bob')
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const path = paths[0].paths_computed[0][0] | |
| if (path.account !== bob.classicAddress) { | |
| throw new Error('Path does not include bob') | |
| } | |
| const paths = response.result.alternatives | |
| if (paths.length === 0) { | |
| throw new Error('No paths found') | |
| } | |
| if ( | |
| !paths[0].paths_computed || | |
| paths[0].paths_computed.length === 0 || | |
| paths[0].paths_computed[0].length === 0 | |
| ) { | |
| throw new Error('No computed paths available') | |
| } | |
| const path = paths[0].paths_computed[0][0] | |
| if (path.account !== bob.classicAddress) { | |
| throw new Error('Path does not include Bob') | |
| } |
packages/xrpl/snippets/src/paths.ts
Outdated
| const sourceAmount = paths[0].source_amount as IssuedCurrencyAmount | ||
| if ( | ||
| sourceAmount.currency !== 'USD' || | ||
| sourceAmount.issuer !== alice.classicAddress || | ||
| // eslint-disable-next-line @typescript-eslint/no-magic-numbers -- 5 is an arbitrarily chosen amount for this test | ||
| parseFloat(sourceAmount.value) !== 5.0 | ||
| ) { | ||
| throw new Error('Unexpected source amount') |
There was a problem hiding this comment.
Ensure source_amount exists before accessing
To prevent runtime errors, verify that source_amount is defined before accessing it.
Suggested change:
+if (!paths[0].source_amount) {
+ throw new Error('Source amount is not defined')
+}
const sourceAmount = paths[0].source_amount as IssuedCurrencyAmount
if (
sourceAmount.currency !== 'USD' ||
sourceAmount.issuer !== alice.classicAddress ||
parseFloat(sourceAmount.value) !== 5.0
) {
throw new Error('Unexpected source amount')
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const sourceAmount = paths[0].source_amount as IssuedCurrencyAmount | |
| if ( | |
| sourceAmount.currency !== 'USD' || | |
| sourceAmount.issuer !== alice.classicAddress || | |
| // eslint-disable-next-line @typescript-eslint/no-magic-numbers -- 5 is an arbitrarily chosen amount for this test | |
| parseFloat(sourceAmount.value) !== 5.0 | |
| ) { | |
| throw new Error('Unexpected source amount') | |
| if (!paths[0].source_amount) { | |
| throw new Error('Source amount is not defined') | |
| } | |
| const sourceAmount = paths[0].source_amount as IssuedCurrencyAmount | |
| if ( | |
| sourceAmount.currency !== 'USD' || | |
| sourceAmount.issuer !== alice.classicAddress || | |
| // eslint-disable-next-line @typescript-eslint/no-magic-numbers -- 5 is an arbitrarily chosen amount for this test | |
| parseFloat(sourceAmount.value) !== 5.0 | |
| ) { | |
| throw new Error('Unexpected source amount') | |
| } |
|
I had to merge these changes into this PR as well: #2773 The CI/CD is not passing without the Paths changes. Once this PR is approved, I can merge the bridge_snippet PR and close this PR. |
perform prerequisite test setup within the test, remove dependence on the external state of testnet
High Level Overview of Change
This is the sister PR for XRPLF/xrpl-py#739. Please check that PR for more context.
Context of Change
The paths snippet tests depends on several test net accounts for its functionality. This PR aims to remove such dependencies. The requirements of the test are setup within the test itself. This causes less disruptions to the CI/CD pipelines after testnet resets.
Type of Change
Did you update HISTORY.md?
Summary by CodeRabbit
New Features
Bug Fixes