refactor: use NetworkID in faucet calculations
#2950
Conversation
WalkthroughThis pull request updates the unreleased section in the HISTORY.md file by clarifying the OracleSet transaction fix and adding an entry for improved faucet support. In the wallet module, it introduces a new constant mapping network IDs to faucet networks, renames constants and functions (e.g., changing Changes
Assessment against linked issues
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/xrpl/test/wallet/fundWallet.test.ts (1)
65-67:⚠️ Potential issueTest needs fixing to pass pipeline
This test is failing in the pipeline. The error indicates that the test expects a function to throw an error, but it's not throwing. With the new NetworkID check, the test logic may need to be updated.
To fix the test, make sure the client doesn't have a networkID set before testing:
it('throws if not connected to a known faucet host', function () { // Info: setupClient.setup creates a connection to 'localhost' + // @ts-expect-error Intentionally modifying private data for test + testContext.client.networkID = undefined assert.throws(() => getFaucetHost(testContext.client)) })🧰 Tools
🪛 GitHub Actions: Node.js CI
[error] 66-66: Expected value to strictly be equal to: undefined. Received: undefined. expected [Function] to throw an error.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/Wallet/defaultFaucets.ts(3 hunks)packages/xrpl/src/Wallet/fundWallet.ts(2 hunks)packages/xrpl/test/wallet/fundWallet.test.ts(2 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
packages/xrpl/src/Wallet/fundWallet.ts (1)
packages/xrpl/src/Wallet/defaultFaucets.ts (1)
getFaucetPath(64-69)
packages/xrpl/test/wallet/fundWallet.test.ts (1)
packages/xrpl/src/Wallet/defaultFaucets.ts (3)
faucetNetworkPaths(19-22)getFaucetPath(64-69)getFaucetHost(36-55)
🪛 GitHub Actions: Node.js CI
packages/xrpl/test/wallet/fundWallet.test.ts
[error] 28-28: Expected value to strictly be equal to: 'faucet.devnet.rippletest.net'. Received: 'faucet.altnet.rippletest.net'.
[error] 66-66: Expected value to strictly be equal to: undefined. Received: undefined. expected [Function] to throw an error.
🔇 Additional comments (11)
packages/xrpl/src/Wallet/defaultFaucets.ts (5)
19-22: Good variable naming improvementRenamed constant from
FaucetNetworkPathstofaucetNetworkPathsto follow JavaScript naming conventions for non-class variables. This is consistent with standard naming practices.
24-27: Good improvement: Using NetworkID for faucet determinationAdded a new
faucetNetworkIDsMap that maps network IDs to faucet networks. This addresses issue #2701 by using NetworkID instead of relying on URL assumptions, making the code more robust.
39-47: Improved error handling and client NetworkID supportThe code now properly checks for the client's networkID before attempting URL-based detection, making the faucet selection process more reliable. This change properly implements the fix requested in issue #2701.
49-52: Good backward compatibilityMaintained the URL-based checks as a fallback mechanism, ensuring backward compatibility with existing client code that may not provide a networkID.
64-69: Improved function namingRenamed
getDefaultFaucetPathtogetFaucetPathfor better clarity and consistency with other getter functions.packages/xrpl/src/Wallet/fundWallet.ts (2)
6-6: Updated import statement correctlyThe import statement has been updated to use the renamed function
getFaucetPathinstead ofgetDefaultFaucetPath, maintaining consistency with the changes in defaultFaucets.ts.
147-147: Function call updated correctlyThe function call has been updated to use
getFaucetPathinstead ofgetDefaultFaucetPath, maintaining consistency with the renaming in defaultFaucets.ts.packages/xrpl/test/wallet/fundWallet.test.ts (2)
5-8: Updated imports correctlyThe import statements have been properly updated to use the renamed entities
faucetNetworkPathsandgetFaucetPath.
61-61: Updated function call correctlyThe function call has been updated to use
getFaucetPathinstead ofgetDefaultFaucetPath.packages/xrpl/HISTORY.md (2)
8-8: Improved documentation clarityUpdated the description to clarify that the
OracleSettransaction change is a fix rather than a new feature, which better reflects the nature of the change.
11-11: Added entry for faucet support improvementsAdded a history entry for "Better faucet support" which aligns with the changes made to use NetworkID for faucet determination rather than URL assumptions.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/xrpl/src/Wallet/defaultFaucets.ts(3 hunks)packages/xrpl/src/Wallet/fundWallet.ts(3 hunks)packages/xrpl/test/wallet/fundWallet.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/xrpl/test/wallet/fundWallet.test.ts
- packages/xrpl/src/Wallet/fundWallet.ts
🧰 Additional context used
🪛 GitHub Actions: Node.js CI
packages/xrpl/src/Wallet/defaultFaucets.ts
[warning] 37-37: Unexpected console statement no-console
🔇 Additional comments (8)
packages/xrpl/src/Wallet/defaultFaucets.ts (8)
19-19: Good rename to follow camelCase conventions.The constant has been appropriately renamed from
FaucetNetworkPathstofaucetNetworkPathsto follow TypeScript naming conventions (camelCase for variables and constants), improving code consistency.
24-27: Good addition of NetworkID to FaucetNetwork mapping.This new mapping enables looking up the appropriate faucet network based on network ID, which aligns with the PR objective of refactoring faucet code to use NetworkID instead of relying on URL assumptions. This will make the code more robust.
38-42: Improved error message with clearer context.The error message has been updated to provide better context about what information is required to create a faucet URL.
44-47: Good implementation of NetworkID-based faucet lookup.This is the core of the refactoring, looking up the faucet network using the client's networkID. This approach is more reliable than inferring from URLs as mentioned in the PR objective (issue #2701).
48-51: Good explicit handling for mainnet.Adding a specific check for network ID 0 (mainnet) with a clear error message improves the API's usability by explicitly informing users that faucets aren't available for mainnet.
61-62: Updated documentation to match renamed constant.The JSDoc has been correctly updated to reference the renamed constant
faucetNetworkPaths.
63-63: Improved function name.The function has been renamed from
getDefaultFaucetPathtogetFaucetPath, which is a better name as it's more concise and doesn't include the redundant "Default" qualifier.
67-68: Updated code to use the renamed constant.The code now correctly references the renamed
faucetNetworkPathsconstant.
| throw new XRPLFaucetError( | ||
| 'Cannot fund an account on an issuing chain. Accounts must be created via the bridge.', | ||
| 'Cannot create faucet URL without networkID or the faucet_host information', |
There was a problem hiding this comment.
| 'Cannot create faucet URL without networkID or the faucet_host information', | |
| 'Cannot create faucet URL without networkID', |
There was a problem hiding this comment.
Why do you want to remove the extra clause? If someone provides the faucet_host, the function doesn't need the network ID.
There was a problem hiding this comment.
How will faucet_host information help? This method will throw an error if client.networkID == null
| `Unable to fund address with faucet after waiting ${ | ||
| INTERVAL_SECONDS * MAX_ATTEMPTS | ||
| } seconds`, | ||
| ) |
There was a problem hiding this comment.
This error message is not relevant for the processSuccessfulResponse method. I do not see any retry-mechanism (or) back-off algorithm used here.
However, it is relevant for getUpdatedBalance method. These two error messages appear to have been swapped.
There was a problem hiding this comment.
No, the errors are correct. The faucet can also be used to add funds to an existing wallet, in which case the error should fall through to here, not throw in getUpdatedBalance.
There was a problem hiding this comment.
Can you point me to the code where we do the waiting (or) retry mechanism? Most of the usages of the faucet have been to generate a new wallet.
Even while funding an existing wallet, the operation is tried exactly once. I don't see the retry-operation.
High Level Overview of Change
This PR refactors the faucet code to use the
NetworkIDto determine which faucet to use.Context of Change
Resolves #2701
Previously, the code was making assumptions about what the URL would be, which isn't very robust.
Type of Change
Did you update HISTORY.md?
Test Plan
CI still passes.