Feat/integrate gem wallet#2
Conversation
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
📝 WalkthroughWalkthroughThis pull request introduces a new GemWallet provider integration for the wallets SDK, including XRPL support with transaction signing, account management, and address formatting utilities. It also adds a debug log statement to the scheduler and exports CAIP formatting utilities from wallets-core. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
| getSupportedChains: (allBlockchains: BlockchainMeta[]) => | ||
| xrplBlockchain(allBlockchains), |
There was a problem hiding this comment.
Typos in variable names and file paths causing runtime errors. In constants.ts, allBlockactions is used instead of allBlockchains, and in mod.ts, singer.js is used instead of signer.js.
This issue appears in multiple locations:
- wallets/provider-gemwallet/src/constants.ts: Lines 29-30
- wallets/provider-gemwallet/src/namespaces/xrpl/mod.ts: Lines 2-2
Please correct all typos in variable names and file paths to match their intended references.
getSupportedChains: (allBlockchains: BlockchainMeta[]) =>
xrplBlockchain(allBlockchains),Prompt for LLM
File wallets/provider-gemwallet/src/constants.ts:
Line 29 to 30:
In the provided TypeScript code, there is a function `getSupportedChains` that takes a parameter named `allBlockchains`. However, inside the function's body, it calls another function `xrplBlockchain` with an argument `allBlockactions`, which is a typo. This will lead to a `ReferenceError` at runtime. Please correct the typo in the argument passed to `xrplBlockchain` to match the parameter name `allBlockchains`.
Suggested Code:
getSupportedChains: (allBlockchains: BlockchainMeta[]) =>
xrplBlockchain(allBlockchains),
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| .removeEventListener((_instance, _callback) => { | ||
| /* | ||
| * TODO: gem wallet doesn't have support for unsubscribing. | ||
| * Making a variable and keep the callback refrence then here make it `undefined` is a quick fix | ||
| * but it makes new bugs, where if two subscribers added at once, we will loose the track of the first one and it will be staled. | ||
| */ | ||
| }) |
There was a problem hiding this comment.
Resource leaks due to improper cleanup. In hooks.ts, removeEventListener is a no-op, and in namespace.ts, the XRPL client is not disconnected on error.
This issue appears in multiple locations:
- wallets/provider-gemwallet/src/namespaces/xrpl/hooks.ts: Lines 27-33
- wallets/provider-gemwallet/src/namespaces/xrpl/namespace.ts: Lines 55-68
Please ensure all resources are properly cleaned up, including event listeners and network connections, especially in error cases.
.removeEventListener((_instance, _callback) => {
// TODO: gem wallet doesn't have support for unsubscribing.
// This feature will cause memory leaks until the underlying API supports listener removal.
console.warn(
'GemWallet provider does not support unsubscribing from "walletChanged" events. This will cause a memory leak.'
);
})Prompt for LLM
File wallets/provider-gemwallet/src/namespaces/xrpl/hooks.ts:
Line 27 to 33:
The provided code implements an event subscriber for GemWallet. The `removeEventListener` function is currently a no-op, as indicated by the TODO comment, because the underlying GemWallet API allegedly does not support unsubscribing from events. This creates a memory leak, as any subscribed listeners will never be cleaned up. This is especially problematic in single-page applications where components subscribe and unsubscribe during their lifecycle. Suggest a way to handle this. Should it throw an error, log a warning, or is there a pattern to manage listeners internally even if the source doesn't support removal?
Suggested Code:
.removeEventListener((_instance, _callback) => {
// TODO: gem wallet doesn't have support for unsubscribing.
// This feature will cause memory leaks until the underlying API supports listener removal.
console.warn(
'GemWallet provider does not support unsubscribing from "walletChanged" events. This will cause a memory leak.'
);
})
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const accountLines = new ActionBuilder<XRPLActions, 'accountLines'>( | ||
| 'accountLines' | ||
| ) | ||
| .action(async (_, account, options) => { |
There was a problem hiding this comment.
The accountLines action uses implicit any types for its parameters, violating type safety rules.
This issue appears in multiple locations:
- wallets/provider-gemwallet/src/namespaces/xrpl/namespace.ts: Lines 55-68
- wallets/provider-gemwallet/src/namespaces/xrpl/helpers.ts: Lines 29-34
- wallets/provider-gemwallet/src/namespaces/xrpl/helpers.ts: Lines 16-23
- wallets/provider-gemwallet/src/utils.ts: Lines 4-16
Please fix this Kody Rule violation in all listed locations.
| .action(async (_, account, options) => { | |
| .action(async (_: unknown, account: string, options?: { peer?: string }) => { |
Warning
This is an experimental feature that generates committable changes. Review the diff before applying. Results may be incorrect.
Prompt for LLM
File wallets/provider-gemwallet/src/namespaces/xrpl/namespace.ts:
Line 55 to 68:
My project enforces a strict rule against using the `any` type to maintain type safety, especially in critical areas like wallet interactions. The following TypeScript code defines an `accountLines` action where the function parameters `_`, `account`, and `options` are implicitly typed as `any`. Refactor this function to include explicit types. From the code's context, the first parameter `_` is unused, `account` is a string representing an address, and `options` is an optional object with a potential `peer` property.
Suggested Code:
.action(async (_: unknown, account: string, options?: { peer?: string }) => {
const client = new Client(XRPL_PUBLIC_SERVER);
await client.connect();
const response = await client.request({
command: 'account_lines',
ledger_index: 'current',
account: account,
peer: options?.peer,
});
await client.disconnect();
return response.result.lines;
})
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| export default async function getSigners(): Promise<SignerFactory> { | ||
| const { Signer: XrplSigner } = await import('./namespaces/xrpl/mod.js'); | ||
| const signers = new DefaultSignerFactory(); | ||
| signers.registerSigner(TxType.XRPL, new XrplSigner()); | ||
| return signers; | ||
| } |
There was a problem hiding this comment.
State isolation bug. The function creates a new DefaultSignerFactory instance on each invocation. This prevents the SignerFactory from acting as a shared singleton, causing any signer registrations in one part of the application to be invisible to others. The dynamic import is also re-evaluated on every call, causing unnecessary overhead. The factory should be initialized only once and reused across all calls.
const signers = (async () => {
const { Signer: XrplSigner } = await import('./namespaces/xrpl/mod.js');
const factory = new DefaultSignerFactory();
factory.registerSigner(TxType.XRPL, new XrplSigner());
return factory;
})();
export default function getSigners(): Promise<SignerFactory> {
return signers;
}Prompt for LLM
File wallets/provider-gemwallet/src/signer.ts:
Line 5 to 10:
The provided TypeScript code defines a function `getSigners` that is supposed to return a `SignerFactory`. However, it creates a new instance of `DefaultSignerFactory` every time it's called. This is a bug because a factory like this is typically expected to be a singleton to share state (like registered signers) across an application. Multiple calls to `getSigners` will result in different, isolated factory instances, leading to inconsistent state and redundant dynamic imports. Please refactor the code to ensure that the `SignerFactory` is initialized only once and that all subsequent calls to `getSigners` return the same instance. The solution should handle the asynchronous dynamic import correctly and efficiently.
Suggested Code:
const signers = (async () => {
const { Signer: XrplSigner } = await import('./namespaces/xrpl/mod.js');
const factory = new DefaultSignerFactory();
factory.registerSigner(TxType.XRPL, new XrplSigner());
return factory;
})();
export default function getSigners(): Promise<SignerFactory> {
return signers;
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| export async function checkInstallationOnLoad(): Promise<boolean> { | ||
| return new Promise((resolve, reject) => { | ||
| window.addEventListener('load', () => { | ||
| isInstalled() | ||
| .then((response) => { | ||
| if (response.result.isInstalled) { | ||
| resolve(true); | ||
| } else { | ||
| resolve(false); | ||
| } | ||
| }) | ||
| .catch(reject); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Race condition on page load. The function adds a window.load event listener without checking if the event has already fired. If checkInstallationOnLoad is called after the page has fully loaded, the listener's callback never runs, and the returned promise never resolves, causing any dependent code to hang indefinitely.
export async function checkInstallationOnLoad(): Promise<boolean> {
return new Promise((resolve, reject) => {
const performCheck = () => {
isInstalled()
.then((response) => {
if (response.result.isInstalled) {
resolve(true);
} else {
resolve(false);
}
})
.catch(reject);
};
if (document.readyState === 'complete') {
performCheck();
} else {
window.addEventListener('load', performCheck);
}
});
}Prompt for LLM
File wallets/provider-gemwallet/src/utils.ts:
Line 3 to 17:
The provided TypeScript function `checkInstallationOnLoad` is intended to check if a wallet is installed when the page loads. It does this by adding a `window.load` event listener. The problem is that it doesn't account for the case where the function is called *after* the `load` event has already fired. In that scenario, the event listener is added too late, its callback never executes, and the Promise it returns never resolves. This will cause any part of the application that awaits this function to hang. Refactor the function to correctly handle this race condition by checking `document.readyState`.
Suggested Code:
export async function checkInstallationOnLoad(): Promise<boolean> {
return new Promise((resolve, reject) => {
const performCheck = () => {
isInstalled()
.then((response) => {
if (response.result.isInstalled) {
resolve(true);
} else {
resolve(false);
}
})
.catch(reject);
};
if (document.readyState === 'complete') {
performCheck();
} else {
window.addEventListener('load', performCheck);
}
});
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
wallets/provider-gemwallet/src/namespaces/xrpl/mod.ts (1)
2-2: Renamesinger.tstosigner.tsfor naming consistency.The file
wallets/provider-gemwallet/src/namespaces/xrpl/singer.tsexports a class namedSigner, creating a mismatch between the filename and the exported entity. This will improve code clarity and maintainability.Update needed
Line 2 in
wallets/provider-gemwallet/src/namespaces/xrpl/mod.ts:export { Signer } from './signer.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallets/provider-gemwallet/src/namespaces/xrpl/mod.ts` at line 2, The export in mod.ts points to the misspelled module; rename the file wallets/provider-gemwallet/src/namespaces/xrpl/singer.ts to signer.ts and update the export statement that references './singer.js' to './signer.js' so it matches the exported class Signer; ensure any other imports or build references use the new filename (signer.ts / signer.js) to avoid broken imports.wallets/provider-gemwallet/src/namespaces/xrpl/singer.ts (1)
12-14: Consider adding a message parameter to align with message-handling signer patterns.While some UTXO-based signers also lack parameters, EVM and Solana implementations consistently accept a message argument. Adding
_msg: stringhere—even though unimplemented—prevents signature misalignment during future refactors and makes the method callable from existing patterns that pass messages.♻️ Suggested change
export class Signer implements GenericSigner<XrplTransaction> { - async signMessage(): Promise<string> { + async signMessage(_msg: string): Promise<string> { throw SignerError.UnimplementedError('signMessage'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallets/provider-gemwallet/src/namespaces/xrpl/singer.ts` around lines 12 - 14, Update the signMessage method signature to accept a message parameter (e.g., _msg: string) so it matches other signer patterns and avoids future call-site mismatches; specifically, modify the async signMessage(): Promise<string> in this file to async signMessage(_msg: string): Promise<string> and keep the current unimplemented behavior (throw SignerError.UnimplementedError('signMessage')) so callers can pass a message without breaking compilation or runtime calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@queue-manager/rango-preset/src/actions/scheduleNextStep.ts`:
- Line 39: Remove the noise-making debug log by deleting the console.log('what
the heck is happening here') call inside the scheduleNextStep function so the
scheduler hot path no longer emits non-actionable messages on every invocation;
if you need diagnostic info keep a guarded logger (e.g., process.env.DEBUG or a
logger.debug call) instead of an unconditional console.log.
In `@wallets/provider-gemwallet/readme.md`:
- Line 5: Replace the non-descriptive link text "here" with a descriptive phrase
that conveys the target, e.g., "implementation status" or "the parent README",
so the sentence reads something like "More about implementation status can be
found in the parent README" (or "More about implementation status can be found
in the implementation status section of the parent README"); update the link
target text in wallets/provider-gemwallet/readme.md accordingly to improve
accessibility and clarity.
In `@wallets/provider-gemwallet/src/namespaces/xrpl/helpers.ts`:
- Around line 16-22: The guard that validates an XRPL Amount incorrectly treats
null as an object and can throw; update the predicates that check the shape of
`amount` (the boolean expression around typeof amount === 'object' and
subsequent checks for amount.currency, amount.issuer, amount.value) to first
ensure amount is not null (e.g., amount !== null && typeof amount === 'object')
before accessing properties, and apply the same null-safe change to the other
guard used at lines 30-33 so both return false for null or malformed inputs
instead of throwing.
In `@wallets/provider-gemwallet/src/namespaces/xrpl/hooks.ts`:
- Around line 24-33: Remove the no-op removeEventListener and add a local
Set-based registry to fan out provider events: create a Set called subscribers
(or similar) in the same module, change addEventListener to add the callback to
subscribers and ensure a single provider listener is registered that calls each
callback in subscribers (use on('walletChanged', providerCallback) once);
implement removeEventListener to delete the callback from subscribers and, if
subscribers becomes empty, stop forwarding (you can keep the provider listener
registered if gemwallet lacks off/unsubscribe, but the providerCallback should
no longer call any callbacks when the set is empty). Reference addEventListener,
removeEventListener, on('walletChanged', callback) and the
providerCallback/subscribers Set when making the change.
In `@wallets/provider-gemwallet/src/namespaces/xrpl/namespace.ts`:
- Around line 36-38: The error message in the XRPL namespace uses the wrong
wallet type; update the thrown Error in namespace.ts (the throw new Error(...)
inside the XRPL namespace connect/init path) to refer to "XRPL wallet" (or a
neutral phrase like "wallet" if preferred) instead of "EVM wallet" so the
message correctly reflects the XRPL context; keep the rest of the message intact
and ensure any similar messages in the same file also use "XRPL" where
appropriate.
- Around line 55-67: The async action handler that creates a Client (new
Client(XRPL_PUBLIC_SERVER)) must ensure client.disconnect() runs even if
client.request(...) throws; wrap the connect/request sequence in a try { await
client.connect(); const response = await client.request(...); return
response.result.lines; } finally { await client.disconnect(); } so disconnect is
always called (use the existing Client, XRPL_PUBLIC_SERVER, .action async
handler and client.connect/request/disconnect symbols).
In `@wallets/provider-gemwallet/src/namespaces/xrpl/singer.ts`:
- Around line 22-25: Import SignerError and SignerErrorCode into this module and
replace the bare Error throws for wallet rejections: where the code currently
does throw new Error('The request has been rejected', { cause: result }) (and
the similar rejection branch), throw new
SignerError(SignerErrorCode.REJECTED_BY_USER, undefined, result) instead so
rejections use the SDK typed error (match the MetaMask/Coin98 signer pattern).
In `@wallets/provider-gemwallet/src/utils.ts`:
- Around line 3-16: checkInstallationOnLoad can hang if called after the page
has already loaded because it waits for the 'load' event; modify it to check
document.readyState first and call isInstalled() immediately when readyState ===
'complete', otherwise attach a one-time load handler; keep the existing behavior
of resolving true/false based on response.result.isInstalled and propagating
rejection from isInstalled; update references to the function name
checkInstallationOnLoad and the called helper isInstalled, and use
window.addEventListener('load', handler, { once: true }) to avoid leaking
listeners.
In `@wallets/readme.md`:
- Line 165: The GemWallet entry in the wallets matrix is misclassified as SUI;
update the table row for "GemWallet" (provider-gemwallet/readme.md) to reflect
the actual integrated chain support used by this PR (XRPL) — either change the
SUI column from ✅ to ❌ and mark the XRPL column as ✅, or add an explicit XRPL
column and set GemWallet to ✅ there; ensure the row accurately matches the
implementation (namespaces/actions referencing XRPL).
---
Nitpick comments:
In `@wallets/provider-gemwallet/src/namespaces/xrpl/mod.ts`:
- Line 2: The export in mod.ts points to the misspelled module; rename the file
wallets/provider-gemwallet/src/namespaces/xrpl/singer.ts to signer.ts and update
the export statement that references './singer.js' to './signer.js' so it
matches the exported class Signer; ensure any other imports or build references
use the new filename (signer.ts / signer.js) to avoid broken imports.
In `@wallets/provider-gemwallet/src/namespaces/xrpl/singer.ts`:
- Around line 12-14: Update the signMessage method signature to accept a message
parameter (e.g., _msg: string) so it matches other signer patterns and avoids
future call-site mismatches; specifically, modify the async signMessage():
Promise<string> in this file to async signMessage(_msg: string): Promise<string>
and keep the current unimplemented behavior (throw
SignerError.UnimplementedError('signMessage')) so callers can pass a message
without breaking compilation or runtime calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 83fec9f4-2d28-4359-a6aa-8e518dcb1cc3
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (20)
queue-manager/rango-preset/src/actions/scheduleNextStep.tswallets/core/src/namespaces/xrpl/mod.tswallets/core/src/namespaces/xrpl/utils.tswallets/provider-all/package.jsonwallets/provider-all/src/index.tswallets/provider-gemwallet/package.jsonwallets/provider-gemwallet/readme.mdwallets/provider-gemwallet/src/constants.tswallets/provider-gemwallet/src/mod.tswallets/provider-gemwallet/src/namespaces/xrpl/helpers.tswallets/provider-gemwallet/src/namespaces/xrpl/hooks.tswallets/provider-gemwallet/src/namespaces/xrpl/mod.tswallets/provider-gemwallet/src/namespaces/xrpl/namespace.tswallets/provider-gemwallet/src/namespaces/xrpl/singer.tswallets/provider-gemwallet/src/provider.tswallets/provider-gemwallet/src/signer.tswallets/provider-gemwallet/src/utils.tswallets/provider-gemwallet/tsconfig.build.jsonwallets/provider-gemwallet/tsconfig.jsonwallets/readme.md
| (step: PendingSwapStep) => step.status === 'failed' | ||
| ); | ||
|
|
||
| console.log('what the heck is happening here'); |
There was a problem hiding this comment.
Remove this debug log from the scheduler hot path.
Line 39 logs on every scheduleNextStep call, which can flood logs and add noise in production. The message is also non-actionable.
Proposed fix
- console.log('what the heck is happening here');📝 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.
| console.log('what the heck is happening here'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@queue-manager/rango-preset/src/actions/scheduleNextStep.ts` at line 39,
Remove the noise-making debug log by deleting the console.log('what the heck is
happening here') call inside the scheduleNextStep function so the scheduler hot
path no longer emits non-actionable messages on every invocation; if you need
diagnostic info keep a guarded logger (e.g., process.env.DEBUG or a logger.debug
call) instead of an unconditional console.log.
| GemWallet integration for hub. | ||
| [Homepage](https://gemwallet.app/) | [Docs](https://gemwallet.app/docs/user-guide/introduction) | ||
|
|
||
| More about implementation status can be found [here](../readme.md). |
There was a problem hiding this comment.
Use descriptive link text instead of “here”.
Line 5 uses non-descriptive link text, which hurts readability/accessibility in docs and rendered link lists.
Suggested doc tweak
-More about implementation status can be found [here](../readme.md).
+More about implementation status can be found in the [wallet providers implementation status](../readme.md).📝 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.
| More about implementation status can be found [here](../readme.md). | |
| More about implementation status can be found in the [wallet providers implementation status](../readme.md). |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 5-5: Link text should be descriptive
(MD059, descriptive-link-text)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wallets/provider-gemwallet/readme.md` at line 5, Replace the non-descriptive
link text "here" with a descriptive phrase that conveys the target, e.g.,
"implementation status" or "the parent README", so the sentence reads something
like "More about implementation status can be found in the parent README" (or
"More about implementation status can be found in the implementation status
section of the parent README"); update the link target text in
wallets/provider-gemwallet/readme.md accordingly to improve accessibility and
clarity.
| return ( | ||
| typeof amount === 'object' && | ||
| // @ts-expect-error it never throw an runtime error, since we are checking it should be an object first | ||
| typeof amount.currency === 'string' && | ||
| // @ts-expect-error it never throw an runtime error, since we are checking it should be an object first | ||
| typeof amount.issuer === 'string' && | ||
| typeof amount.value === 'string' |
There was a problem hiding this comment.
Make the XRPL amount guards null-safe.
typeof null === 'object', so both guards can still throw on malformed input before the caller reaches its fallback error path. These should return false, not crash.
🛠️ Suggested change
function isIssuedCurrencyAmount(
amount: XrplPaymentTransactionData['Amount']
): amount is XrplTransactionDataIssuedCurrencyAmount {
- return (
- typeof amount === 'object' &&
- // `@ts-expect-error` it never throw an runtime error, since we are checking it should be an object first
- typeof amount.currency === 'string' &&
- // `@ts-expect-error` it never throw an runtime error, since we are checking it should be an object first
- typeof amount.issuer === 'string' &&
- typeof amount.value === 'string'
- );
+ return !!amount &&
+ typeof amount === 'object' &&
+ 'currency' in amount &&
+ typeof amount.currency === 'string' &&
+ 'issuer' in amount &&
+ typeof amount.issuer === 'string' &&
+ 'value' in amount &&
+ typeof amount.value === 'string';
}
function isMPTokenAmount(
amount: XrplPaymentTransactionData['Amount']
): amount is XrplTransactionDataMPTAmount {
- return (
- typeof amount === 'object' &&
- // `@ts-expect-error` it never throw an runtime error, since we are checking it should be an object first
- typeof amount.mpt_issuance_id === 'string' &&
- typeof amount.value === 'string'
- );
+ return !!amount &&
+ typeof amount === 'object' &&
+ 'mpt_issuance_id' in amount &&
+ typeof amount.mpt_issuance_id === 'string' &&
+ 'value' in amount &&
+ typeof amount.value === 'string';
}Also applies to: 30-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wallets/provider-gemwallet/src/namespaces/xrpl/helpers.ts` around lines 16 -
22, The guard that validates an XRPL Amount incorrectly treats null as an object
and can throw; update the predicates that check the shape of `amount` (the
boolean expression around typeof amount === 'object' and subsequent checks for
amount.currency, amount.issuer, amount.value) to first ensure amount is not null
(e.g., amount !== null && typeof amount === 'object') before accessing
properties, and apply the same null-safe change to the other guard used at lines
30-33 so both return false for null or malformed inputs instead of throwing.
| .addEventListener((_, callback) => { | ||
| on('walletChanged', callback); | ||
| }) | ||
| .removeEventListener((_instance, _callback) => { | ||
| /* | ||
| * TODO: gem wallet doesn't have support for unsubscribing. | ||
| * Making a variable and keep the callback refrence then here make it `undefined` is a quick fix | ||
| * but it makes new bugs, where if two subscribers added at once, we will loose the track of the first one and it will be staled. | ||
| */ | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does @gemwallet/apiversion 3.8.0 provide an official way to unsubscribe or remove listeners registered viaon('walletChanged', callback)?
💡 Result:
No, @gemwallet/api version 3.8.0 does not provide an official way to unsubscribe or remove listeners registered via on('walletChanged', callback). Official documentation across multiple versions (including 3.6.0, 3.7.0, and earlier) shows examples of using on to register listeners for events like walletChanged, but no mention of an off, removeListener, or unsubscribe method. The npm package files list various helpers and methods but no explicit event management beyond registration. Source code is in the GitHub repo (GemWallet/gemwallet-extension), but no snippets indicate unsubscription support.
Citations:
- 1: https://www.npmjs.com/package/@gemwallet/api
- 2: https://registry.npmjs.org/%40gemwallet%2Fapi
- 3: https://gemwallet.app/docs/3.6.0/api/gemwallet-api-reference
- 4: https://gemwallet.app/docs/api/gemwallet-api-reference
- 5: https://github.com/GemWallet/gemwallet-extension
Remove no-op removeEventListener and implement local callback registry to prevent subscriber accumulation.
@gemwallet/api does not support unsubscribing from listeners, so the current no-op removeEventListener at line 27 can accumulate callbacks across repeated subscribe/unsubscribe cycles, causing stale and duplicate updates.
Implement a local Set-based fan-out registry to manage subscribers independently of the provider's single registration:
Suggested implementation
+const walletChangedSubscribers = new Set<(payload: WalletChangedEventPayload) => void>();
+let bridgeRegistered = false;
+
+const walletChangedBridge = (payload: WalletChangedEventPayload) => {
+ for (const cb of walletChangedSubscribers) cb(payload);
+};
+
export function changeAccountSubscriberBuilder() {
return new ChangeAccountSubscriberBuilder<
WalletChangedEventPayload,
true,
XRPLActions
>()
.getInstance(() => true)
.format(async (_, payload) => [
utils.formatAddressToCAIP(payload.wallet.publicAddress),
])
.addEventListener((_, callback) => {
- on('walletChanged', callback);
+ if (!bridgeRegistered) {
+ on('walletChanged', walletChangedBridge);
+ bridgeRegistered = true;
+ }
+ walletChangedSubscribers.add(callback);
})
- .removeEventListener((_instance, _callback) => {
- /*
- * TODO: gem wallet doesn't have support for unsubscribing.
- * Making a variable and keep the callback refrence then here make it `undefined` is a quick fix
- * but it makes new bugs, where if two subscribers added at once, we will loose the track of the first one and it will be staled.
- */
+ .removeEventListener((_instance, callback) => {
+ walletChangedSubscribers.delete(callback);
})
.build();
}📝 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.
| .addEventListener((_, callback) => { | |
| on('walletChanged', callback); | |
| }) | |
| .removeEventListener((_instance, _callback) => { | |
| /* | |
| * TODO: gem wallet doesn't have support for unsubscribing. | |
| * Making a variable and keep the callback refrence then here make it `undefined` is a quick fix | |
| * but it makes new bugs, where if two subscribers added at once, we will loose the track of the first one and it will be staled. | |
| */ | |
| }) | |
| const walletChangedSubscribers = new Set<(payload: WalletChangedEventPayload) => void>(); | |
| let bridgeRegistered = false; | |
| const walletChangedBridge = (payload: WalletChangedEventPayload) => { | |
| for (const cb of walletChangedSubscribers) cb(payload); | |
| }; | |
| export function changeAccountSubscriberBuilder() { | |
| return new ChangeAccountSubscriberBuilder< | |
| WalletChangedEventPayload, | |
| true, | |
| XRPLActions | |
| >() | |
| .getInstance(() => true) | |
| .format(async (_, payload) => [ | |
| utils.formatAddressToCAIP(payload.wallet.publicAddress), | |
| ]) | |
| .addEventListener((_, callback) => { | |
| if (!bridgeRegistered) { | |
| on('walletChanged', walletChangedBridge); | |
| bridgeRegistered = true; | |
| } | |
| walletChangedSubscribers.add(callback); | |
| }) | |
| .removeEventListener((_instance, callback) => { | |
| walletChangedSubscribers.delete(callback); | |
| }) | |
| .build(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wallets/provider-gemwallet/src/namespaces/xrpl/hooks.ts` around lines 24 -
33, Remove the no-op removeEventListener and add a local Set-based registry to
fan out provider events: create a Set called subscribers (or similar) in the
same module, change addEventListener to add the callback to subscribers and
ensure a single provider listener is registered that calls each callback in
subscribers (use on('walletChanged', providerCallback) once); implement
removeEventListener to delete the callback from subscribers and, if subscribers
becomes empty, stop forwarding (you can keep the provider listener registered if
gemwallet lacks off/unsubscribe, but the providerCallback should no longer call
any callbacks when the set is empty). Reference addEventListener,
removeEventListener, on('walletChanged', callback) and the
providerCallback/subscribers Set when making the change.
| throw new Error( | ||
| 'Trying to eagerly connect to your EVM wallet, but seems its instance is not available.' | ||
| ); |
There was a problem hiding this comment.
Fix wallet type in error message.
Line 37 says “EVM wallet” in an XRPL namespace path; this is misleading for users and debugging.
Suggested text fix
- 'Trying to eagerly connect to your EVM wallet, but seems its instance is not available.'
+ 'Trying to eagerly connect to your XRPL wallet, but its instance is not available.'📝 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.
| throw new Error( | |
| 'Trying to eagerly connect to your EVM wallet, but seems its instance is not available.' | |
| ); | |
| throw new Error( | |
| 'Trying to eagerly connect to your XRPL wallet, but its instance is not available.' | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wallets/provider-gemwallet/src/namespaces/xrpl/namespace.ts` around lines 36
- 38, The error message in the XRPL namespace uses the wrong wallet type; update
the thrown Error in namespace.ts (the throw new Error(...) inside the XRPL
namespace connect/init path) to refer to "XRPL wallet" (or a neutral phrase like
"wallet" if preferred) instead of "EVM wallet" so the message correctly reflects
the XRPL context; keep the rest of the message intact and ensure any similar
messages in the same file also use "XRPL" where appropriate.
| .action(async (_, account, options) => { | ||
| const client = new Client(XRPL_PUBLIC_SERVER); | ||
| await client.connect(); | ||
|
|
||
| const response = await client.request({ | ||
| command: 'account_lines', | ||
| ledger_index: 'current', | ||
| account: account, | ||
| peer: options?.peer, | ||
| }); | ||
|
|
||
| await client.disconnect(); | ||
| return response.result.lines; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect other XRPL namespace implementations for client lifecycle pattern consistency.
rg -n -C3 'new Client\(|client\.connect\(|client\.disconnect\(' --type=tsRepository: RyukTheCoder/rango-client
Length of output: 3090
🏁 Script executed:
# Check the full namespace file to see all action handlers and client usage patterns
cat -n wallets/provider-gemwallet/src/namespaces/xrpl/namespace.tsRepository: RyukTheCoder/rango-client
Length of output: 2687
🏁 Script executed:
# Search for all client lifecycle patterns in the XRPL namespace directory
rg -n 'new Client|\.connect\(|\.disconnect\(' wallets/provider-gemwallet/src/namespaces/xrpl/ --type=ts -C2Repository: RyukTheCoder/rango-client
Length of output: 1440
Wrap client request in try-finally to ensure disconnect on errors.
If client.request() at line 59 throws, the disconnect() call is skipped, leaving the connection open.
Proposed fix
.action(async (_, account, options) => {
const client = new Client(XRPL_PUBLIC_SERVER);
- await client.connect();
-
- const response = await client.request({
- command: 'account_lines',
- ledger_index: 'current',
- account: account,
- peer: options?.peer,
- });
-
- await client.disconnect();
- return response.result.lines;
+ await client.connect();
+ try {
+ const response = await client.request({
+ command: 'account_lines',
+ ledger_index: 'current',
+ account: account,
+ peer: options?.peer,
+ });
+ return response.result.lines;
+ } finally {
+ await client.disconnect();
+ }
})📝 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.
| .action(async (_, account, options) => { | |
| const client = new Client(XRPL_PUBLIC_SERVER); | |
| await client.connect(); | |
| const response = await client.request({ | |
| command: 'account_lines', | |
| ledger_index: 'current', | |
| account: account, | |
| peer: options?.peer, | |
| }); | |
| await client.disconnect(); | |
| return response.result.lines; | |
| .action(async (_, account, options) => { | |
| const client = new Client(XRPL_PUBLIC_SERVER); | |
| await client.connect(); | |
| try { | |
| const response = await client.request({ | |
| command: 'account_lines', | |
| ledger_index: 'current', | |
| account: account, | |
| peer: options?.peer, | |
| }); | |
| return response.result.lines; | |
| } finally { | |
| await client.disconnect(); | |
| } | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wallets/provider-gemwallet/src/namespaces/xrpl/namespace.ts` around lines 55
- 67, The async action handler that creates a Client (new
Client(XRPL_PUBLIC_SERVER)) must ensure client.disconnect() runs even if
client.request(...) throws; wrap the connect/request sequence in a try { await
client.connect(); const response = await client.request(...); return
response.result.lines; } finally { await client.disconnect(); } so disconnect is
always called (use the existing Client, XRPL_PUBLIC_SERVER, .action async
handler and client.connect/request/disconnect symbols).
| if (result.type === 'reject') { | ||
| throw new Error('The request has been rejected', { | ||
| cause: result, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect signer rejection/error handling patterns"
rg -n -C3 --type=ts 'SignerError|SignerErrorCode|UserRejected|type === .reject.' walletsRepository: RyukTheCoder/rango-client
Length of output: 29645
🏁 Script executed:
rg -n 'SignerErrorCode' wallets/provider-gemwalletRepository: RyukTheCoder/rango-client
Length of output: 51
🏁 Script executed:
rg -n 'REJECTED_BY_USER' walletsRepository: RyukTheCoder/rango-client
Length of output: 326
🏁 Script executed:
head -20 wallets/provider-gemwallet/src/namespaces/xrpl/singer.tsRepository: RyukTheCoder/rango-client
Length of output: 785
🏁 Script executed:
rg -n 'from.*rango-types' wallets/provider-gemwallet/src/namespaces/xrpl/singer.tsRepository: RyukTheCoder/rango-client
Length of output: 195
🏁 Script executed:
rg -n 'SignerErrorCode.*from.*rango-types' walletsRepository: RyukTheCoder/rango-client
Length of output: 1154
Use SignerError with REJECTED_BY_USER code for wallet rejections.
Both rejection branches (lines 22–25 and 42–45) throw a bare Error instead of the SDK's typed error. Update the import to include SignerErrorCode and throw SignerError(SignerErrorCode.REJECTED_BY_USER, undefined, result) instead, matching the pattern established by MetaMask and Coin98 signers.
Updated import (line 4)
- import { type GenericSigner, SignerError } from 'rango-types';
+ import { type GenericSigner, SignerError, SignerErrorCode } from 'rango-types';Updated rejection handling (lines 22–25 and 42–45)
if (result.type === 'reject') {
- throw new Error('The request has been rejected', {
- cause: result,
- });
+ throw new SignerError(SignerErrorCode.REJECTED_BY_USER, undefined, result);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wallets/provider-gemwallet/src/namespaces/xrpl/singer.ts` around lines 22 -
25, Import SignerError and SignerErrorCode into this module and replace the bare
Error throws for wallet rejections: where the code currently does throw new
Error('The request has been rejected', { cause: result }) (and the similar
rejection branch), throw new SignerError(SignerErrorCode.REJECTED_BY_USER,
undefined, result) instead so rejections use the SDK typed error (match the
MetaMask/Coin98 signer pattern).
| export async function checkInstallationOnLoad(): Promise<boolean> { | ||
| return new Promise((resolve, reject) => { | ||
| window.addEventListener('load', () => { | ||
| isInstalled() | ||
| .then((response) => { | ||
| if (response.result.isInstalled) { | ||
| resolve(true); | ||
| } else { | ||
| resolve(false); | ||
| } | ||
| }) | ||
| .catch(reject); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all call sites affected by potential unresolved promise.
rg -n -C3 '\bcheckInstallationOnLoad\s*\(' --type=tsRepository: RyukTheCoder/rango-client
Length of output: 1786
checkInstallationOnLoad hangs indefinitely when called after page load.
If this function runs after document.readyState is already complete, the load event listener never fires and the promise remains pending. This blocks state updates in provider.ts and hangs the eager-connect action in namespaces/xrpl/namespace.ts.
Proposed fix
export async function checkInstallationOnLoad(): Promise<boolean> {
- return new Promise((resolve, reject) => {
- window.addEventListener('load', () => {
- isInstalled()
- .then((response) => {
- if (response.result.isInstalled) {
- resolve(true);
- } else {
- resolve(false);
- }
- })
- .catch(reject);
- });
- });
+ const check = async () => {
+ const response = await isInstalled();
+ return !!response.result?.isInstalled;
+ };
+
+ if (document.readyState === 'complete') {
+ return check();
+ }
+
+ return new Promise((resolve, reject) => {
+ window.addEventListener(
+ 'load',
+ () => {
+ check().then(resolve).catch(reject);
+ },
+ { once: true }
+ );
+ });
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wallets/provider-gemwallet/src/utils.ts` around lines 3 - 16,
checkInstallationOnLoad can hang if called after the page has already loaded
because it waits for the 'load' event; modify it to check document.readyState
first and call isInstalled() immediately when readyState === 'complete',
otherwise attach a one-time load handler; keep the existing behavior of
resolving true/false based on response.result.isInstalled and propagating
rejection from isInstalled; update references to the function name
checkInstallationOnLoad and the called helper isInstalled, and use
window.addEventListener('load', handler, { once: true }) to avoid leaking
listeners.
| | [Xverse](provider-xverse/readme.md) | ❌ | ⚠️ | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | | ||
| | [Tomo](provider-tomo/readme.md) | ✅ | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | | ||
| | [Coin98](provider-coin98/readme.md) | ✅ | ❌ | ✅ | ❌ | ❌ | ❌ | ❌ | ❌ | | ||
| | [GemWallet](provider-gemwallet/readme.md) | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | ✅ | ❌ | |
There was a problem hiding this comment.
GemWallet chain-group capability appears misclassified.
Line 165 marks GemWallet as SUI ✅, but this PR integrates GemWallet via XRPL namespaces/actions. Please align the matrix with the implemented chain support (or add explicit XRPL representation in this table) to avoid incorrect integration decisions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wallets/readme.md` at line 165, The GemWallet entry in the wallets matrix is
misclassified as SUI; update the table row for "GemWallet"
(provider-gemwallet/readme.md) to reflect the actual integrated chain support
used by this PR (XRPL) — either change the SUI column from ✅ to ❌ and mark the
XRPL column as ✅, or add an explicit XRPL column and set GemWallet to ✅ there;
ensure the row accurately matches the implementation (namespaces/actions
referencing XRPL).
Review Summary by QodoIntegrate GemWallet provider for XRPL blockchain support
WalkthroughsDescription• Integrates GemWallet as new XRPL wallet provider with full transaction support • Adds CAIP-2 address formatting utility for XRPL namespace • Implements TrustSet and Payment transaction signing via GemWallet • Detects and handles account changes from GemWallet • Updates wallet provider registry and documentation Diagramflowchart LR
GW["GemWallet Extension"]
API["@gemwallet/api"]
PROV["provider-gemwallet"]
XRPL["XRPL Namespace"]
SIGN["Transaction Signer"]
CORE["wallets-core"]
ALL["provider-all Registry"]
GW -->|"API calls"| API
API -->|"imported by"| PROV
PROV -->|"implements"| XRPL
XRPL -->|"contains"| SIGN
XRPL -->|"uses utils from"| CORE
PROV -->|"registered in"| ALL
File Changes1. wallets/provider-gemwallet/src/constants.ts
|
Code Review by Qodo
1. Stray debug log
|
| console.log('what the heck is happening here'); | ||
|
|
There was a problem hiding this comment.
1. Stray debug log 🐞 Bug ⚙ Maintainability
scheduleNextStep now prints a hardcoded debug message on every execution, creating noisy production logs and potentially leaking internal behavior during swaps.
Agent Prompt
### Issue description
A stray `console.log` was committed and will run on every call to `scheduleNextStep`, generating noisy logs.
### Issue Context
This code runs in the queue-manager execution loop.
### Fix Focus Areas
- queue-manager/rango-preset/src/actions/scheduleNextStep.ts[39-40]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| export async function checkInstallationOnLoad(): Promise<boolean> { | ||
| return new Promise((resolve, reject) => { | ||
| window.addEventListener('load', () => { | ||
| isInstalled() | ||
| .then((response) => { | ||
| if (response.result.isInstalled) { | ||
| resolve(true); | ||
| } else { | ||
| resolve(false); | ||
| } | ||
| }) | ||
| .catch(reject); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
2. Install check can hang 🐞 Bug ☼ Reliability
checkInstallationOnLoad only resolves inside a window 'load' event handler; if called after the load event has already fired, the returned Promise never resolves, blocking provider init and canEagerConnect.
Agent Prompt
### Issue description
`checkInstallationOnLoad()` only resolves when the `window` emits `'load'`. In SPAs or when the provider initializes after page load, that event has already fired and the Promise never resolves.
### Issue Context
This function is awaited by `canEagerConnect` and used in provider `.init()` to set `installed` state.
### Fix Focus Areas
- wallets/provider-gemwallet/src/utils.ts[3-16]
- wallets/provider-gemwallet/src/namespaces/xrpl/namespace.ts[33-38]
- wallets/provider-gemwallet/src/provider.ts[9-15]
### Implementation direction
- If `document.readyState === 'complete'` (or not `'loading'`), call `isInstalled()` immediately.
- Otherwise, add a `'load'` listener with `{ once: true }` and resolve after calling `isInstalled()`.
- Ensure the Promise always resolves/rejects.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| .addEventListener((_, callback) => { | ||
| on('walletChanged', callback); | ||
| }) | ||
| .removeEventListener((_instance, _callback) => { | ||
| /* | ||
| * TODO: gem wallet doesn't have support for unsubscribing. | ||
| * Making a variable and keep the callback refrence then here make it `undefined` is a quick fix | ||
| * but it makes new bugs, where if two subscribers added at once, we will loose the track of the first one and it will be staled. | ||
| */ | ||
| }) |
There was a problem hiding this comment.
3. Account listener not removable 🐞 Bug ☼ Reliability
GemWallet's account-change subscriber registers a walletChanged listener but provides a no-op removeEventListener, so cleanup cannot detach callbacks and repeated subscriptions can accumulate and duplicate state updates.
Agent Prompt
### Issue description
The GemWallet account-change subscription cannot be cleaned up: `addEventListener` registers a listener, but `removeEventListener` is a no-op. Core cleanup calls `removeEventListener`, so callbacks can leak and duplicate.
### Issue Context
GemWallet API may not support true unsubscribe, but we still need to prevent duplicated callbacks and stale state updates.
### Fix Focus Areas
- wallets/provider-gemwallet/src/namespaces/xrpl/hooks.ts[13-34]
- wallets/core/src/namespaces/common/hooks/changeAccountSubscriber.ts[210-227]
### Implementation direction
- Prefer returning an unsubscribe function from `.addEventListener(...)` if GemWallet `on(...)` can provide one.
- If GemWallet cannot unsubscribe, implement a single underlying `walletChanged` listener and multiplex to a `Set` of subscribers; `removeEventListener` should delete from the set so cleanup becomes effective (even if the underlying listener remains).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| .action(async (_, account, options) => { | ||
| const client = new Client(XRPL_PUBLIC_SERVER); | ||
| await client.connect(); | ||
|
|
||
| const response = await client.request({ | ||
| command: 'account_lines', | ||
| ledger_index: 'current', | ||
| account: account, | ||
| peer: options?.peer, | ||
| }); | ||
|
|
||
| await client.disconnect(); | ||
| return response.result.lines; |
There was a problem hiding this comment.
4. Xrpl client leak on error 🐞 Bug ☼ Reliability
accountLines connects an xrpl Client and disconnects only on the success path; if connect() or request() throws, the websocket may remain open and leak resources.
Agent Prompt
### Issue description
`accountLines` does not guarantee `client.disconnect()` will run when `connect()` or `request()` throws.
### Issue Context
This can leak websocket connections and degrade long-running sessions.
### Fix Focus Areas
- wallets/provider-gemwallet/src/namespaces/xrpl/namespace.ts[55-67]
### Implementation direction
Wrap connect/request in `try { ... } finally { await client.disconnect().catch(() => {}) }` (and consider guarding `disconnect` if `connect` fails).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Integrates GemWallet as a new wallet provider, enabling support for XRPL blockchain interactions.
Key changes include:
provider-gemwalletmodule, making GemWallet available as a wallet option.TrustSetandPaymenttransactions on the XRPL network via GemWallet.AccountIdstrings.walletsREADME and adds a dedicatedreadme.mdfor the GemWallet provider, detailing its features and known limitations (e.g., nosignMessagesupport, limited auto-connect, no MPT support for XRPL payments).