Conversation
📝 WalkthroughWalkthroughAdds a full XRPL platform to the monorepo: new platform package, types, address/chain implementations, signer, unsigned transaction type, RPC config update to wss, SDK integration and exports, build/test configs, and unit/integration tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Signer as XrplSigner
participant Client as XRPL_Client
participant Ledger as XRP_Ledger
User->>Signer: sign(transactions[])
activate Signer
Signer->>Client: connect() if not connected
activate Client
Client->>Ledger: open websocket / establish session
Ledger-->>Client: connection established
Client-->>Signer: connected
deactivate Client
loop per transaction
Signer->>Client: autofill(tx)
activate Client
Client->>Ledger: request state (fees, sequence, etc.)
Ledger-->>Client: state
Client-->>Signer: autofilled tx
deactivate Client
Signer->>Signer: sign with Wallet.fromSeed -> signed blob
end
Signer->>User: return SignedTx[] (blobs)
deactivate Signer
sequenceDiagram
participant User
participant Signer as XrplSigner
participant Client as XRPL_Client
participant Ledger as XRP_Ledger
User->>Signer: signAndSend(transactions[])
activate Signer
Signer->>Client: connect() if needed
activate Client
Client->>Ledger: open websocket
Ledger-->>Client: connected
Client-->>Signer: connected
deactivate Client
loop per transaction
Signer->>Client: autofill(tx)
activate Client
Client->>Ledger: fetch state
Ledger-->>Client: state
Client-->>Signer: autofilled tx
deactivate Client
Signer->>Signer: sign with Wallet
Signer->>Client: submit(signedTx)
activate Client
Client->>Ledger: submit tx
Ledger-->>Client: tx hash & provisional status
Client-->>Signer: tx hash
deactivate Client
Signer->>Client: waitForTransaction(txHash)
activate Client
Client->>Ledger: poll/subscribe for finality
Ledger-->>Client: confirmed
Client-->>Signer: confirmed
deactivate Client
end
Signer->>User: return TxHash[] (64-char hex)
deactivate Signer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
platforms/xrpl/src/types.ts (1)
15-15: Consider narrowingAnyXrplAddresstoUniversalOrXrpl | stringifUint8Arrayis not a first-class XRPL address representation.Looking at the analogous type in other platforms (e.g., EVM, Solana),
Uint8Arrayis typically included only when the platform natively uses raw byte addresses (e.g., for 32-byte public keys). XRPL uses base58-check encoded account addresses (r...), so acceptingUint8Arrayhere could introduce silent misuse at call sites without validation inXrplAddress.Verify whether
platforms/xrpl/src/address.tsactually handlesUint8Arrayinput before keeping this in the union.#!/bin/bash # Description: Check if XrplAddress constructor handles Uint8Array input. rg -n "Uint8Array" platforms/xrpl/src/address.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/xrpl/src/types.ts` at line 15, The type AnyXrplAddress currently includes Uint8Array but XRPL addresses are base58-check strings; inspect XrplAddress (in platforms/xrpl/src/address.ts) for explicit handling/conversion/validation of Uint8Array inputs—if XrplAddress does not accept or convert Uint8Array, remove Uint8Array from the AnyXrplAddress union in types.ts to prevent silent misuse; if it does accept them, ensure the constructor/method (XrplAddress) normalizes/validates the Uint8Array to a canonical base58 address and add tests demonstrating that path.platforms/xrpl/package.json (1)
54-60:typesVersionsonly maps to the index entry — the./addresssub-export has no entry.Consumers on older TypeScript versions (< 4.7) that rely on
typesVersionsrather thanexportsconditions won't resolve types for@wormhole-foundation/sdk-xrpl/address. Add an entry for the address sub-path if you intend to support those consumers.Proposed fix
"typesVersions": { "*": { - "*": [ + ".": [ "dist/cjs/index.d.ts" + ], + "address": [ + "dist/cjs/address.d.ts" ] } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/xrpl/package.json` around lines 54 - 60, The typesVersions mapping only points to "dist/cjs/index.d.ts" so the address sub-export (`@wormhole-foundation/sdk-xrpl/address`) has no type entry; update the package.json typesVersions section to add a "./address" key mapping to the built declaration for that subpath (e.g., point "./address" to "dist/cjs/address.d.ts" or the correct emitted .d.ts file), ensuring the typesVersions object covers both the root index and the address subpath so older TypeScript consumers resolve types for the address export.platforms/xrpl/src/signer.ts (3)
26-28:_networkis accepted but discarded.The constructor parameter
_network: Networklacks theprivatemodifier, so it is not stored. IfSignAndSendSigner(or any future code) needs the network, this will silently fail. Either store it or remove it if truly unused.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/xrpl/src/signer.ts` around lines 26 - 28, The constructor currently accepts _network: Network but doesn't store it (missing the private modifier), so either remove the parameter if unused or persist it for later use; update the constructor signature in the Signer class to include a private (or readonly) modifier for _network (e.g., private _network: Network) so instances can access the network, and adjust any references in methods like SignAndSendSigner to use this._network; if you choose to remove it, also delete any references and tests expecting the parameter.
13-18: ParameterprivateKeyis actually an XRPL seed — rename for clarity.
Wallet.fromSeed()expects a seed string, not a private key. The current naming is misleading and could lead consumers to pass an actual private key (hex) instead of an XRPL seed (e.g.,sEd...).Proposed rename
export async function getXrplSigner( rpc: Client, - privateKey: string, + seed: string, ): Promise<SignAndSendSigner<Network, XrplChains>> { const [network, chain] = await XrplPlatform.chainFromRpc(rpc); - return new XrplSigner(chain, network, rpc, privateKey); + return new XrplSigner(chain, network, rpc, seed); }constructor( private _chain: C, _network: Network, private _provider: Client, - privateKey: string, + seed: string, ) { - this._wallet = Wallet.fromSeed(privateKey); + this._wallet = Wallet.fromSeed(seed); }Also applies to: 26-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/xrpl/src/signer.ts` around lines 13 - 18, The parameter named privateKey is actually an XRPL seed; rename it to seed in the getXrplSigner function signature and all related usages to avoid confusion (e.g., change privateKey -> seed in getXrplSigner and the other function at lines 26-33), and update any calls into XrplSigner or Wallet.fromSeed to pass the renamed seed variable; ensure parameter types and JSDoc/comments reflect "seed (XRPL seed string)" and that XrplSigner construction and Wallet.fromSeed invocations use the new seed identifier.
35-51: Misleading variable nameconnected— it actually means "we opened the connection".
const connected = !this._provider.isConnected()istruewhen the provider was not connected, meaning "we need to connect (and later disconnect)." This inverted semantics is error-prone and appears in bothsignAndSendandsign.Proposed rename
- const connected = !this._provider.isConnected(); - if (connected) await this._provider.connect(); + const needsConnect = !this._provider.isConnected(); + if (needsConnect) await this._provider.connect(); ... - if (connected) await this._provider.disconnect(); + if (needsConnect) await this._provider.disconnect();Apply the same rename in
sign()(Lines 55-68) and inplatform.tsLines 97-107 and 138-150 where the identical pattern is used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/xrpl/src/signer.ts` around lines 35 - 51, The local boolean variable currently named connected has inverted semantics (it is true when the provider was NOT connected), so rename it to a clear name like needToConnect or shouldConnect in signAndSend and sign to reflect "we need to open the connection (and later close it)"; update both the variable declaration and its usage in the connect()/disconnect() logic and ensure the finally block uses the new name, and apply the same rename and adjustments to the identical pattern in the Platform class methods in platform.ts so all checks and awaits remain logically consistent.platforms/xrpl/src/platform.ts (2)
89-111:getBalancefor non-native tokens throws outside the connection management block — verify this is intentional.Line 110 throws after the
if (isNative(token))block, meaning therpcconnection is never opened for non-native tokens. This is correct (no leak), but the control flow is subtle. A guard clause at the top would make intent clearer:Suggested restructure
static async getBalance<C extends XrplChains>( _network: Network, _chain: C, rpc: Client, walletAddr: string, token: TokenAddress<C>, ): Promise<bigint | null> { + if (!isNative(token)) { + throw new Error("Token balance lookup not yet implemented for XRPL"); + } - if (isNative(token)) { - const connected = !rpc.isConnected(); - if (connected) await rpc.connect(); - try { - const response = await rpc.request({ - command: "account_info", - account: walletAddr, - ledger_index: "validated", - }); - return BigInt(response.result.account_data.Balance); - } finally { - if (connected) await rpc.disconnect(); - } - } - throw new Error("Token balance lookup not yet implemented for XRPL"); + const needsConnect = !rpc.isConnected(); + if (needsConnect) await rpc.connect(); + try { + const response = await rpc.request({ + command: "account_info", + account: walletAddr, + ledger_index: "validated", + }); + return BigInt(response.result.account_data.Balance); + } finally { + if (needsConnect) await rpc.disconnect(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/xrpl/src/platform.ts` around lines 89 - 111, The control flow in getBalance is subtle because the non-native token error is thrown after the native-only branch, which can confuse reviewers; refactor getBalance by adding an early guard clause using isNative(token) (or its negation) so non-native tokens immediately throw before any rpc.connect/disconnect logic, and keep the existing connection management (the connected check, rpc.connect, try/finally with rpc.disconnect) inside the native-token branch; reference getBalance, isNative, rpc.connect, rpc.disconnect, and the thrown Error message to locate and update the logic.
38-41:getRpccreates a new disconnectedClienton every call.Each invocation returns a fresh
Clientinstance that is not connected. This means every consumer must manageconnect()/disconnect()lifecycles, and there's no connection reuse. This is workable but worth documenting — callers that forget to connect will get confusing errors.platforms/xrpl/__tests__/unit/address.test.ts (1)
3-57: Good baseline coverage. Consider adding a few edge-case tests.The tests cover the main API surface well. A couple of gaps worth adding:
equals()with aUniversalAddress— exercises theelsebranch ataddress.tsLine 67.equals()returningfalsefor two different addresses — confirms inequality logic.XrplAddress.instanceof()— the static type guard used internally.These would improve branch coverage without much effort.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/xrpl/__tests__/unit/address.test.ts` around lines 3 - 57, Add three unit tests to exercise the equals and instanceof branches: (1) a test that calls XrplAddress.equals with a UniversalAddress instance (create a UniversalAddress from a known XrplAddress and assert XrplAddress.equals(universal) returns true) to hit the else-branch in XrplAddress.equals; (2) a test that constructs two different XrplAddress values (two distinct valid addresses) and asserts equals returns false to verify inequality logic; and (3) a test for the static type guard XrplAddress.instanceof by passing an object shaped like an address and a non-address and asserting true/false accordingly so the instanceof guard is covered. Ensure you use the existing helpers: XrplAddress, UniversalAddress, XrplZeroAddress (or another valid addresses), and the class methods equals(), toUniversalAddress(), and instanceof() in your assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@platforms/xrpl/__tests__/integration/platform.test.ts`:
- Around line 9-11: Replace the hardcoded always-funded testnet account by
funding a fresh wallet in the test setup: in the test file's beforeAll, call the
XRPL client.fundWallet() (as used in signer.test.ts) to create and store a
funded wallet instead of using TESTNET_ACCOUNT; update tests to reference the
funded wallet variable (remove reliance on the TESTNET_ACCOUNT constant) so the
suite is self-contained and survives ledger resets.
In `@platforms/xrpl/__tests__/integration/signer.test.ts`:
- Around line 55-75: The test for XrplSigner.signAndSend is using a hardcoded
Destination that may not exist on Testnet, causing tecNO_DST; modify the test
setup to create and fund a second wallet in the beforeAll (e.g.,
secondFundedWallet) and use secondFundedWallet.classicAddress as the Destination
in the XrplUnsignedTransaction used by the test, or alternatively send an amount
>= base reserve from the fundedWallet to that destination to ensure the account
is activated before calling signAndSend; update any references in the test to
use the new funded wallet variable and keep XrplSigner and signAndSend behavior
unchanged.
In `@platforms/xrpl/package.json`:
- Around line 49-52: The xrpl dependency is using a caret range ("^4.2.0") that
can pull in compromised 4.2.x releases; update the dependency in package.json to
either pin to "4.2.0" exactly or bump to a safe range (e.g., "^4.2.5" or later)
and then run CI/build to verify no breaking changes; also audit usages of xrpl
APIs in this package—specifically references to Client, Wallet.fromSeed,
isValidClassicAddress, decodeAccountID, and SubmittableTransaction—to ensure
those symbols exist and behave the same in the chosen xrpl version, updating
imports or call sites if needed.
In `@platforms/xrpl/src/address.ts`:
- Around line 59-61: The static instanceof method on XrplAddress dereferences
address (address.constructor.platform) and will throw for null/undefined; update
XrplAddress.instanceof to first guard that address is a non-null object and that
address.constructor exists before comparing its platform to XrplAddress.platform
(e.g., check address !== null && typeof address === 'object' &&
address.constructor && address.constructor.platform === XrplAddress.platform) so
that null/undefined or non-object inputs safely return false rather than
throwing.
- Around line 15-29: The constructor currently accepts AnyXrplAddress but only
handles XrplAddress instances and strings; update the constructor to also accept
Uint8Array by converting the bytes back to a classic XRPL address (use your
project’s account ID/address encode helper or xrpl library helper) and then
validate with XrplAddress.isValidAddress before assigning to this.address; keep
throwing on other types. Also harden XrplAddress.instanceof by guarding access
to address.constructor and address.constructor.platform (e.g., check address !=
null and address.constructor != null) before comparing platform to avoid runtime
errors when unexpected values are passed.
In `@platforms/xrpl/src/platform.ts`:
- Around line 137-152: chainFromRpc currently treats any non-zero network_id as
"Testnet", which misclassifies Devnet (network_id = 2); update chainFromRpc to
explicitly map known network_id values to networks: treat network_id === 0 or
network_id === undefined as "Mainnet", network_id === 2 as "Devnet", and return
"Testnet" for other known testnet ids (or default to "Testnet") — implement this
mapping in the function (use networkId variable and the chainFromRpc function)
via a switch/if-else so Devnet is returned correctly.
---
Nitpick comments:
In `@platforms/xrpl/__tests__/unit/address.test.ts`:
- Around line 3-57: Add three unit tests to exercise the equals and instanceof
branches: (1) a test that calls XrplAddress.equals with a UniversalAddress
instance (create a UniversalAddress from a known XrplAddress and assert
XrplAddress.equals(universal) returns true) to hit the else-branch in
XrplAddress.equals; (2) a test that constructs two different XrplAddress values
(two distinct valid addresses) and asserts equals returns false to verify
inequality logic; and (3) a test for the static type guard
XrplAddress.instanceof by passing an object shaped like an address and a
non-address and asserting true/false accordingly so the instanceof guard is
covered. Ensure you use the existing helpers: XrplAddress, UniversalAddress,
XrplZeroAddress (or another valid addresses), and the class methods equals(),
toUniversalAddress(), and instanceof() in your assertions.
In `@platforms/xrpl/package.json`:
- Around line 54-60: The typesVersions mapping only points to
"dist/cjs/index.d.ts" so the address sub-export
(`@wormhole-foundation/sdk-xrpl/address`) has no type entry; update the
package.json typesVersions section to add a "./address" key mapping to the built
declaration for that subpath (e.g., point "./address" to "dist/cjs/address.d.ts"
or the correct emitted .d.ts file), ensuring the typesVersions object covers
both the root index and the address subpath so older TypeScript consumers
resolve types for the address export.
In `@platforms/xrpl/src/platform.ts`:
- Around line 89-111: The control flow in getBalance is subtle because the
non-native token error is thrown after the native-only branch, which can confuse
reviewers; refactor getBalance by adding an early guard clause using
isNative(token) (or its negation) so non-native tokens immediately throw before
any rpc.connect/disconnect logic, and keep the existing connection management
(the connected check, rpc.connect, try/finally with rpc.disconnect) inside the
native-token branch; reference getBalance, isNative, rpc.connect,
rpc.disconnect, and the thrown Error message to locate and update the logic.
In `@platforms/xrpl/src/signer.ts`:
- Around line 26-28: The constructor currently accepts _network: Network but
doesn't store it (missing the private modifier), so either remove the parameter
if unused or persist it for later use; update the constructor signature in the
Signer class to include a private (or readonly) modifier for _network (e.g.,
private _network: Network) so instances can access the network, and adjust any
references in methods like SignAndSendSigner to use this._network; if you choose
to remove it, also delete any references and tests expecting the parameter.
- Around line 13-18: The parameter named privateKey is actually an XRPL seed;
rename it to seed in the getXrplSigner function signature and all related usages
to avoid confusion (e.g., change privateKey -> seed in getXrplSigner and the
other function at lines 26-33), and update any calls into XrplSigner or
Wallet.fromSeed to pass the renamed seed variable; ensure parameter types and
JSDoc/comments reflect "seed (XRPL seed string)" and that XrplSigner
construction and Wallet.fromSeed invocations use the new seed identifier.
- Around line 35-51: The local boolean variable currently named connected has
inverted semantics (it is true when the provider was NOT connected), so rename
it to a clear name like needToConnect or shouldConnect in signAndSend and sign
to reflect "we need to open the connection (and later close it)"; update both
the variable declaration and its usage in the connect()/disconnect() logic and
ensure the finally block uses the new name, and apply the same rename and
adjustments to the identical pattern in the Platform class methods in
platform.ts so all checks and awaits remain logically consistent.
In `@platforms/xrpl/src/types.ts`:
- Line 15: The type AnyXrplAddress currently includes Uint8Array but XRPL
addresses are base58-check strings; inspect XrplAddress (in
platforms/xrpl/src/address.ts) for explicit handling/conversion/validation of
Uint8Array inputs—if XrplAddress does not accept or convert Uint8Array, remove
Uint8Array from the AnyXrplAddress union in types.ts to prevent silent misuse;
if it does accept them, ensure the constructor/method (XrplAddress)
normalizes/validates the Uint8Array to a canonical base58 address and add tests
demonstrating that path.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
core/base/src/constants/rpc.tspackage.jsonplatforms/xrpl/__tests__/integration/platform.test.tsplatforms/xrpl/__tests__/integration/signer.test.tsplatforms/xrpl/__tests__/unit/address.test.tsplatforms/xrpl/__tests__/unit/signer.test.tsplatforms/xrpl/jest.config.tsplatforms/xrpl/package.jsonplatforms/xrpl/src/address.tsplatforms/xrpl/src/chain.tsplatforms/xrpl/src/index.tsplatforms/xrpl/src/platform.tsplatforms/xrpl/src/signer.tsplatforms/xrpl/src/types.tsplatforms/xrpl/src/unsignedTransaction.tsplatforms/xrpl/tsconfig.cjs.jsonplatforms/xrpl/tsconfig.esm.jsonsdk/package.jsonsdk/src/addresses.tssdk/src/platforms/xrpl.tssdk/src/xrpl.tssdk/tsconfig.cjs.jsonsdk/tsconfig.esm.json
Comment on lines
+9
to
+11
| // Well-known genesis / faucet-funded testnet account | ||
| // (always exists, always has a balance) | ||
| const TESTNET_ACCOUNT = "rPT1Sjq2YGrBMTttX4GZHjKu9dyfzbpAYe"; |
There was a problem hiding this comment.
Testnet account may not survive ledger resets.
rPT1Sjq2YGrBMTttX4GZHjKu9dyfzbpAYe is described as "always exists, always has a balance," but the XRPL testnet is periodically reset, which deletes all accounts. Consider funding a fresh wallet via client.fundWallet() in beforeAll (as done in signer.test.ts) to make the test self-contained and resilient to resets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platforms/xrpl/__tests__/integration/platform.test.ts` around lines 9 - 11,
Replace the hardcoded always-funded testnet account by funding a fresh wallet in
the test setup: in the test file's beforeAll, call the XRPL client.fundWallet()
(as used in signer.test.ts) to create and store a funded wallet instead of using
TESTNET_ACCOUNT; update tests to reference the funded wallet variable (remove
reliance on the TESTNET_ACCOUNT constant) so the suite is self-contained and
survives ledger resets.
Comment on lines
+55
to
+75
| it("signAndSend() submits a payment and returns a tx hash", async () => { | ||
| const signer = new XrplSigner("Xrpl", "Testnet", client, fundedWallet.seed!); | ||
|
|
||
| const unsignedTx = new XrplUnsignedTransaction( | ||
| { | ||
| TransactionType: "Payment", | ||
| Account: fundedWallet.classicAddress, | ||
| Destination: "rPT1Sjq2YGrBMTttX4GZHjKu9dyfzbpAYe", | ||
| Amount: "1000000", // 1 XRP in drops | ||
| }, | ||
| "Testnet", | ||
| "Xrpl", | ||
| "Test payment sign-and-send", | ||
| ); | ||
|
|
||
| const hashes = await signer.signAndSend([unsignedTx]); | ||
| expect(hashes).toHaveLength(1); | ||
| expect(typeof hashes[0]).toBe("string"); | ||
| // XRPL tx hashes are 64 hex chars | ||
| expect(hashes[0]).toMatch(/^[A-F0-9]{64}$/); | ||
| }); |
There was a problem hiding this comment.
signAndSend test depends on the destination account existing on testnet.
XRPL requires the destination of a Payment to be an existing (activated) account unless the amount meets the reserve requirement. If rPT1Sjq2YGrBMTttX4GZHjKu9dyfzbpAYe is deleted during a testnet reset, this test will fail with a tecNO_DST error. Consider either:
- Funding a second wallet in
beforeAlland using it as the destination, or - Sending enough XRP to activate the account (≥ base reserve) to handle the case where the account doesn't exist.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platforms/xrpl/__tests__/integration/signer.test.ts` around lines 55 - 75,
The test for XrplSigner.signAndSend is using a hardcoded Destination that may
not exist on Testnet, causing tecNO_DST; modify the test setup to create and
fund a second wallet in the beforeAll (e.g., secondFundedWallet) and use
secondFundedWallet.classicAddress as the Destination in the
XrplUnsignedTransaction used by the test, or alternatively send an amount >=
base reserve from the fundedWallet to that destination to ensure the account is
activated before calling signAndSend; update any references in the test to use
the new funded wallet variable and keep XrplSigner and signAndSend behavior
unchanged.
Comment on lines
+49
to
+52
| "dependencies": { | ||
| "xrpl": "^4.2.0", | ||
| "@wormhole-foundation/sdk-connect": "1.20.0" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what version of sdk-connect other platform packages use
rg '"@wormhole-foundation/sdk-connect"' --type=json -g 'platforms/*/package.json' -A 0Repository: wormhole-foundation/wormhole-sdk-ts
Length of output: 710
🌐 Web query:
xrpl npm package version 4.2.0 availability and compatibility
💡 Result:
- Availability:
xrplv4.2.0 is a published, installable npm release (it has its own packagedpackage.jsonwith"version": "4.2.0"). [1] - Compatibility (runtime):
xrpl@4.2.0declares Node.js>=18.0.0viaengines, and it’s intended for Node.js and the browser (has abrowserfield and ships UMD bundles viaunpkg/jsdelivr). [1] - What changed in 4.2.0: The
xrpl@4.2.0GitHub release notes list additions like DynamicNFT support, simulate RPC (XLS-69d), PermissionedDomain (XLS-80d), Deep Freeze (XLS-77d), and AMMClawback support. [2] - Security / version selection:
xrpl@4.2.0is not in the known-compromised range (4.2.1–4.2.4) from the April 2025 supply-chain incident; patched safe versions were released afterward (e.g., 4.2.5+). If you can, prefer upgrading beyond 4.2.0 (at least 4.2.5). [3][4]
Sources: [1] [2] [3] [4]
Pin xrpl to safe version and verify API compatibility.
The @wormhole-foundation/sdk-connect version (1.20.0) is correctly aligned across all platforms in the monorepo, and xrpl@4.2.0 is a published, compatible release. However, the caret range ^4.2.0 allows installation of versions 4.2.1–4.2.4, which were compromised in a documented supply-chain incident in April 2025. Either pin to exactly "4.2.0" or upgrade to "^4.2.5" or later. Additionally, verify that the APIs used in this package (Client, Wallet.fromSeed, isValidClassicAddress, decodeAccountID, SubmittableTransaction) are available and unchanged in the chosen version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platforms/xrpl/package.json` around lines 49 - 52, The xrpl dependency is
using a caret range ("^4.2.0") that can pull in compromised 4.2.x releases;
update the dependency in package.json to either pin to "4.2.0" exactly or bump
to a safe range (e.g., "^4.2.5" or later) and then run CI/build to verify no
breaking changes; also audit usages of xrpl APIs in this package—specifically
references to Client, Wallet.fromSeed, isValidClassicAddress, decodeAccountID,
and SubmittableTransaction—to ensure those symbols exist and behave the same in
the chosen xrpl version, updating imports or call sites if needed.
Comment on lines
+15
to
+29
| constructor(address: AnyXrplAddress) { | ||
| if (XrplAddress.instanceof(address)) { | ||
| const a = address as unknown as XrplAddress; | ||
| this.address = a.address; | ||
| return; | ||
| } | ||
|
|
||
| if (typeof address === "string") { | ||
| if (!XrplAddress.isValidAddress(address)) { | ||
| throw new Error(`Invalid XRPL address: ${address}`); | ||
| } | ||
| this.address = address; | ||
| } else { | ||
| throw new Error(`Invalid XRPL address: ${address}`); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Uint8Array is ever passed to XrplAddress constructor
rg -n "new XrplAddress" --type=ts -C3Repository: wormhole-foundation/wormhole-sdk-ts
Length of output: 4053
🏁 Script executed:
#!/bin/bash
# Check the AnyXrplAddress type definition
fd "types.ts" --type=f -path "*/xrpl/*" | head -5Repository: wormhole-foundation/wormhole-sdk-ts
Length of output: 247
🏁 Script executed:
#!/bin/bash
# Examine the address.ts file to understand the full constructor and instanceof implementation
cat -n platforms/xrpl/src/address.ts | head -80Repository: wormhole-foundation/wormhole-sdk-ts
Length of output: 2847
🏁 Script executed:
#!/bin/bash
# Search for AnyXrplAddress type definition and usage
rg -n "AnyXrplAddress" --type=ts -B2 -A2Repository: wormhole-foundation/wormhole-sdk-ts
Length of output: 1038
🏁 Script executed:
#!/bin/bash
# Check UniversalOrXrpl type to understand what XrplAddress instances include
rg -n "UniversalOrXrpl" --type-add "ts:*.ts" -t ts -B5 -A5Repository: wormhole-foundation/wormhole-sdk-ts
Length of output: 568
🏁 Script executed:
#!/bin/bash
# Search for any Uint8Array usage specifically with XrplAddress or address construction
rg -n "Uint8Array" platforms/xrpl/ --type-add "ts:*.ts" -t ts -C3Repository: wormhole-foundation/wormhole-sdk-ts
Length of output: 2206
🏁 Script executed:
#!/bin/bash
# Check if instanceof is called with null/undefined anywhere or if there are guards
rg -n "instanceof" platforms/xrpl/src/ --type-add "ts:*.ts" -t ts -B2 -A2Repository: wormhole-foundation/wormhole-sdk-ts
Length of output: 1038
🏁 Script executed:
#!/bin/bash
# Look at the imports and types to understand the full picture
cat -n platforms/xrpl/src/types.tsRepository: wormhole-foundation/wormhole-sdk-ts
Length of output: 639
Constructor does not handle Uint8Array despite AnyXrplAddress including it.
AnyXrplAddress is typed as UniversalOrXrpl | string | Uint8Array (types.ts line 15), but the constructor only handles XrplAddress instances and string. Passing a Uint8Array falls into the else branch and throws. Either narrow AnyXrplAddress to exclude Uint8Array, or add handling for it (e.g., encode the account ID back to a classic address).
Additionally, the instanceof method (line 59) accesses address.constructor.platform without guarding against null or undefined. While the type signature doesn't include these, add a safety check to prevent runtime errors if the method is called with unexpected values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platforms/xrpl/src/address.ts` around lines 15 - 29, The constructor
currently accepts AnyXrplAddress but only handles XrplAddress instances and
strings; update the constructor to also accept Uint8Array by converting the
bytes back to a classic XRPL address (use your project’s account ID/address
encode helper or xrpl library helper) and then validate with
XrplAddress.isValidAddress before assigning to this.address; keep throwing on
other types. Also harden XrplAddress.instanceof by guarding access to
address.constructor and address.constructor.platform (e.g., check address !=
null and address.constructor != null) before comparing platform to avoid runtime
errors when unexpected values are passed.
Comment on lines
+59
to
+61
| static instanceof(address: any): address is XrplAddress { | ||
| return address.constructor.platform === XrplAddress.platform; | ||
| } |
There was a problem hiding this comment.
instanceof will throw on null/undefined input.
address.constructor.platform dereferences address without a null guard. If null or undefined is passed, this throws an unintended TypeError instead of returning false.
Proposed fix
static instanceof(address: any): address is XrplAddress {
+ if (address == null || typeof address !== "object") return false;
return address.constructor.platform === XrplAddress.platform;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platforms/xrpl/src/address.ts` around lines 59 - 61, The static instanceof
method on XrplAddress dereferences address (address.constructor.platform) and
will throw for null/undefined; update XrplAddress.instanceof to first guard that
address is a non-null object and that address.constructor exists before
comparing its platform to XrplAddress.platform (e.g., check address !== null &&
typeof address === 'object' && address.constructor &&
address.constructor.platform === XrplAddress.platform) so that null/undefined or
non-object inputs safely return false rather than throwing.
Comment on lines
137
to
152
| static async chainFromRpc(rpc: Client): Promise<[Network, XrplChains]> { | ||
| const connected = !rpc.isConnected(); | ||
| if (connected) await rpc.connect(); | ||
| try { | ||
| const response = await rpc.request({ | ||
| command: "server_info", | ||
| }); | ||
| const networkId = response.result.info.network_id; | ||
| if (networkId === 0 || networkId === undefined) { | ||
| return ["Mainnet", "Xrpl"]; | ||
| } | ||
| return ["Testnet", "Xrpl"]; | ||
| } finally { | ||
| if (connected) await rpc.disconnect(); | ||
| } | ||
| } |
There was a problem hiding this comment.
chainFromRpc network detection may misclassify Devnet.
network_id === 0 || undefined → Mainnet, everything else → Testnet. XRPL Devnet (typically network_id = 2) would be returned as "Testnet". If Devnet support is planned, this needs a more robust mapping.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platforms/xrpl/src/platform.ts` around lines 137 - 152, chainFromRpc
currently treats any non-zero network_id as "Testnet", which misclassifies
Devnet (network_id = 2); update chainFromRpc to explicitly map known network_id
values to networks: treat network_id === 0 or network_id === undefined as
"Mainnet", network_id === 2 as "Devnet", and return "Testnet" for other known
testnet ids (or default to "Testnet") — implement this mapping in the function
(use networkId variable and the chainFromRpc function) via a switch/if-else so
Devnet is returned correctly.
emreboga
reviewed
Feb 25, 2026
emreboga
reviewed
Feb 25, 2026
emreboga
reviewed
Feb 25, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@platforms/xrpl/src/signer.ts`:
- Around line 35-67: The signAndSend and sign methods currently unconditionally
disconnect the provider in their finally blocks which can tear down
caller-managed/shared connections; change both methods to track whether this
method actually opened the connection (e.g., a local flag set when calling
this._provider.connect()) and only call this._provider.disconnect() in the
finally block if that flag is true and the provider is still connected, leaving
externally-managed connections intact; update both signAndSend and sign to use
this "openedHere" flag around the existing connect() and disconnect() calls.
- Around line 13-33: Rename the parameter named privateKey to seed in both
getXrplSigner and the XrplSigner constructor to reflect that Wallet.fromSeed()
expects an XRPL Base58Check seed string; update parameter names and any internal
references (_wallet initialization using Wallet.fromSeed(seed)) and adjust
exported function signature and class constructor signature accordingly
(getXrplSigner, XrplSigner, and the Wallet.fromSeed call) so the API clearly
communicates a seed is required rather than a raw hex private key.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
platforms/xrpl/src/platform.tsplatforms/xrpl/src/signer.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- platforms/xrpl/src/platform.ts
Comment on lines
+13
to
+33
| export async function getXrplSigner( | ||
| rpc: Client, | ||
| privateKey: string, | ||
| ): Promise<SignAndSendSigner<Network, XrplChains>> { | ||
| const [network, chain] = await XrplPlatform.chainFromRpc(rpc); | ||
| return new XrplSigner(chain, network, rpc, privateKey); | ||
| } | ||
|
|
||
| export class XrplSigner<N extends Network, C extends XrplChains> | ||
| implements SignAndSendSigner<N, C> | ||
| { | ||
| private readonly _wallet: Wallet; | ||
|
|
||
| constructor( | ||
| private _chain: C, | ||
| _network: Network, | ||
| private _provider: Client, | ||
| privateKey: string, | ||
| ) { | ||
| this._wallet = Wallet.fromSeed(privateKey); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
XRPL npm package Wallet.fromSeed() method what format accepts seed private key
💡 Result:
Wallet.fromSeed() (aka Wallet.fromSecret) in the xrpl npm package expects an XRPL “seed” / “secret” string in XRPL Base58Check format (not a raw private key). The docs describe it as a “base58 encoded seed”, with examples like ssZkdwURFMBXenJPbrpE14b6noJSu (secp256k1 seeds also commonly start with s...). [1][2]
So:
- Accepted input: XRPL Base58Check seed value (“seed/secret”), which per XRPL encoding conventions starts with
s. [2] - Not accepted: a hex private key (e.g.,
00...33-byte hex) or other private-key formats. XRPL “seed” and “private key” are different concepts; software usually derives the private key from the seed. [3]
If you only have a private key, you generally need to construct a wallet from a keypair (publicKey + privateKey) rather than fromSeed, or convert from other material (mnemonic/entropy) into a seed using the appropriate utilities (e.g., encodeSeed/decodeSeed live in ripple-address-codec). [1][4]
Sources:
[1] xrpl.js Wallet docs (Wallet.fromSeed, “base58 encoded seed”)
[2] XRPL base58 encodings (seed values start with s, Base58Check)
[3] XRPL cryptographic keys (seed vs private key)
[4] ripple-address-codec docs (encodeSeed / decodeSeed)
Rename privateKey to seed to match Wallet creation semantics.
At lines 15 and 30, the parameter is named privateKey, but line 32 uses Wallet.fromSeed(), which expects an XRPL Base58Check seed string (e.g., ssZkdwURFMBXenJPbrpE14b6noJSu), not a raw hex private key. This naming mismatch creates an incorrect API contract and invites misuse by developers who may pass a raw private key instead of a seed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platforms/xrpl/src/signer.ts` around lines 13 - 33, Rename the parameter
named privateKey to seed in both getXrplSigner and the XrplSigner constructor to
reflect that Wallet.fromSeed() expects an XRPL Base58Check seed string; update
parameter names and any internal references (_wallet initialization using
Wallet.fromSeed(seed)) and adjust exported function signature and class
constructor signature accordingly (getXrplSigner, XrplSigner, and the
Wallet.fromSeed call) so the API clearly communicates a seed is required rather
than a raw hex private key.
Comment on lines
+35
to
+67
| async signAndSend(txs: UnsignedTransaction<N, C>[]): Promise<TxHash[]> { | ||
| if (!this._provider.isConnected()) await this._provider.connect(); | ||
|
|
||
| try { | ||
| const txHashes: TxHash[] = []; | ||
| for (const tx of txs) { | ||
| const xrplTx = tx as XrplUnsignedTransaction<N, C>; | ||
| const prepared = await this._provider.autofill(xrplTx.transaction); | ||
| const signed = this._wallet.sign(prepared); | ||
| const result = await this._provider.submitAndWait(signed.tx_blob); | ||
| txHashes.push(result.result.hash); | ||
| } | ||
| return txHashes; | ||
| } finally { | ||
| if (this._provider.isConnected()) await this._provider.disconnect(); | ||
| } | ||
| } | ||
|
|
||
| async sign(txs: UnsignedTransaction<N, C>[]): Promise<SignedTx[]> { | ||
| if (!this._provider.isConnected()) await this._provider.connect(); | ||
|
|
||
| try { | ||
| const signedTxs: SignedTx[] = []; | ||
| for (const tx of txs) { | ||
| const xrplTx = tx as XrplUnsignedTransaction<N, C>; | ||
| const prepared = await this._provider.autofill(xrplTx.transaction); | ||
| const signed = this._wallet.sign(prepared); | ||
| signedTxs.push(signed.tx_blob); | ||
| } | ||
| return signedTxs; | ||
| } finally { | ||
| if (this._provider.isConnected()) await this._provider.disconnect(); | ||
| } |
There was a problem hiding this comment.
Preserve caller-owned RPC connection state instead of always disconnecting.
At Line 36/54 you may connect conditionally, but at Line 49/66 you always disconnect if connected. This tears down caller-managed/shared connections and can break concurrent usage.
🔧 Proposed fix
async signAndSend(txs: UnsignedTransaction<N, C>[]): Promise<TxHash[]> {
- if (!this._provider.isConnected()) await this._provider.connect();
+ const shouldDisconnect = !this._provider.isConnected();
+ if (shouldDisconnect) await this._provider.connect();
try {
const txHashes: TxHash[] = [];
for (const tx of txs) {
const xrplTx = tx as XrplUnsignedTransaction<N, C>;
const prepared = await this._provider.autofill(xrplTx.transaction);
const signed = this._wallet.sign(prepared);
const result = await this._provider.submitAndWait(signed.tx_blob);
txHashes.push(result.result.hash);
}
return txHashes;
} finally {
- if (this._provider.isConnected()) await this._provider.disconnect();
+ if (shouldDisconnect && this._provider.isConnected()) await this._provider.disconnect();
}
}
async sign(txs: UnsignedTransaction<N, C>[]): Promise<SignedTx[]> {
- if (!this._provider.isConnected()) await this._provider.connect();
+ const shouldDisconnect = !this._provider.isConnected();
+ if (shouldDisconnect) await this._provider.connect();
try {
const signedTxs: SignedTx[] = [];
for (const tx of txs) {
const xrplTx = tx as XrplUnsignedTransaction<N, C>;
const prepared = await this._provider.autofill(xrplTx.transaction);
const signed = this._wallet.sign(prepared);
signedTxs.push(signed.tx_blob);
}
return signedTxs;
} finally {
- if (this._provider.isConnected()) await this._provider.disconnect();
+ if (shouldDisconnect && this._provider.isConnected()) await this._provider.disconnect();
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platforms/xrpl/src/signer.ts` around lines 35 - 67, The signAndSend and sign
methods currently unconditionally disconnect the provider in their finally
blocks which can tear down caller-managed/shared connections; change both
methods to track whether this method actually opened the connection (e.g., a
local flag set when calling this._provider.connect()) and only call
this._provider.disconnect() in the finally block if that flag is true and the
provider is still connected, leaving externally-managed connections intact;
update both signAndSend and sign to use this "openedHere" flag around the
existing connect() and disconnect() calls.
kev1n-peters
approved these changes
Feb 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Packaging / Chores
Tests