wallets: merge main into wallets-v1#1659
wallets: merge main into wallets-v1#1659guilleasz-crossmint wants to merge 10 commits intowallets-v1from
Conversation
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
* a * change * fix
* a * chaneg
…ated orders (#1647) * Add orderId + clientSecret support to hosted checkout * Simplify URL param appending for hosted checkout v3 * Make fiat and crypto payment configs optional for existing orders * Encode orderId in URL path to handle special characters securely
* Feat: add server key signers * added solana and stellar support, cleaned up changes * added changeset * added tests * update lockfile * lint fix * address Greptile review comments: hex validation, signer checks, docs Co-Authored-By: Jonathan Derbyshire <jonathan.temp@paella.dev> * fix: replace any cast with unknown, add base64 validation for stellar signer Co-Authored-By: Jonathan Derbyshire <jonathan.temp@paella.dev> * fix: enforce 64-char minimum secret length in key derivation Co-Authored-By: Jonathan Derbyshire <jonathan.temp@paella.dev> * fixed txn signing, and cleanup * fix lint * fix tests * lint fix * revert some signer wallet logic and cleanup * reverted alias stuff * cleanup * add pragmatic server side env check * fix messed up previous commits * update schema to assume backend wallet changes * fix lint * update schema to align with backend changes * add server signer type to delegate signer * lint fix * improved delegated signer check for server signers * review feedback * reverted type in openapi.json * fix type issue breaking change revert --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Resolved conflicts in 11 files: - packages/wallets/src/api/__tests__/test-utils.ts - packages/wallets/src/api/types.ts - packages/wallets/src/openapi.json - packages/wallets/src/signers/types.ts - packages/wallets/src/wallets/evm.ts - packages/wallets/src/wallets/solana.ts - packages/wallets/src/wallets/stellar.ts - packages/wallets/src/wallets/types.ts - packages/wallets/src/wallets/wallet-factory.test.ts - packages/wallets/src/wallets/wallet-factory.ts - packages/wallets/src/wallets/wallet.ts Merge strategy: - Keep wallets-v1 API surface (signer instead of experimental_signer, recovery instead of signer for admin) - Integrate main's server signer support into wallets-v1's architecture - Add resolveSigner helper to Wallet class for transparent device/server signer handling - Update tests to use new API conventions Co-Authored-By: Guille <guille.a@paella.dev>
…et factory - Add server signer handling to validateSigners() with deriveServerSignerDetails - Add server signer handling to registerSigners() to resolve server:derivedAddress locators - Skip compareSignerConfigs for server signers (API returns external-wallet type) - Fix server signer locator return type in evm/solana/stellar server signers - Fix missing DeviceSignerKeyStorage import and syntax error in wallet-factory - Update test for getWallet server signer validation (not applicable in wallets-v1 API) Co-Authored-By: Guille <guille.a@paella.dev>
Original prompt from Guille
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
…ivityResponseUnstableDto The Transfers type (WalletsActivityResponseUnstableDto) has .events not .data. Updated the quickstart-devkit demo app to use the correct property names. Co-Authored-By: Guille <guille.a@paella.dev>
Co-Authored-By: Guille <guille.a@paella.dev>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 4 potential issues.
⚠️ 1 issue in files not directly in the diff
⚠️ getSignerLocator has no handler for server signer type, falls through to generic return (packages/wallets/src/utils/signer-locator.ts:33)
In signer-locator.ts:33, getSignerLocator has no case for type === "server". If a ServerSignerConfig reaches this function (e.g., if the chain != null guard in wallet-factory.ts:382 is somehow bypassed), it would fall through to return signer.type as SignerLocator which returns the string "server" — an invalid signer locator that would cause API errors. While the current call site in registerSigners guards with chain != null, this is fragile since getSignerLocator is a shared utility that could be called from other locations.
View 5 additional findings in Devin Review.
There was a problem hiding this comment.
🟡 buildInternalSignerConfig throws for server signer type in recovery path
In wallet.ts:1044-1045, the buildInternalSignerConfig switch has no case for "server" and hits the default throw new Error("Unknown signer type: server"). When createWalletInstance (wallet-factory.ts:184) passes the API response's admin signer config as recovery to the Wallet constructor, and that signer has type: "server", any code path that calls buildInternalSignerConfig with the recovery config will throw. Currently this is only called for device signers, but it creates a latent incompatibility where server-signer wallets cannot be extended with device signers.
(Refers to lines 1044-1045)
Was this helpful? React with 👍 or 👎 to provide feedback.
| const { orderId, ...restOfParams } = props as CrossmintHostedCheckoutV3OrderProps; | ||
| appendObjectToQueryParams(queryParams, restOfParams); |
There was a problem hiding this comment.
🔴 clientSecret leaked in URL query parameters for order-based checkout
When using the order-based checkout flow, the clientSecret is included in the URL query string. In crossmintHostedCheckoutV3Service.ts:31, only orderId is destructured out of the props before passing the rest to appendObjectToQueryParams. This means clientSecret is appended as a visible query parameter in the popup/new-tab URL. Query parameters are stored in browser history, can appear in server logs, referrer headers, and browser extensions. The clientSecret should either be excluded from query params or transmitted through a more secure channel (e.g., postMessage after the window opens).
Was this helpful? React with 👍 or 👎 to provide feedback.
| } | ||
|
|
||
| async signMessage(message: string) { | ||
| if (!/^[A-Za-z0-9+/]*={0,2}$/.test(message)) { |
There was a problem hiding this comment.
🟡 Stellar base64 validation regex accepts empty strings
In stellar-server-signer.ts:25, the regex /^[A-Za-z0-9+/]*={0,2}$/ matches an empty string because * allows zero characters and ={0,2} allows zero padding. An empty message would pass validation and be signed, producing a signature over an empty buffer, which is likely unintended and could cause downstream issues.
| if (!/^[A-Za-z0-9+/]*={0,2}$/.test(message)) { | |
| if (!message || !/^[A-Za-z0-9+/]+={0,2}$/.test(message)) { |
Was this helpful? React with 👍 or 👎 to provide feedback.
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/wallets/src/wallets/wallet-factory.ts
Line: 382-391
Comment:
**Silent fallthrough produces invalid server signer locator**
When `signer.type === "server"` but `chain` is `null` / `undefined`, the condition on line 382 is false and execution falls through to `getSignerLocator(signer)`. `getSignerLocator` has no special handling for `"server"`, so it hits the catch-all `return signer.type as SignerLocator` and returns the bare string `"server"` instead of `"server:<derivedAddress>"`. This silently submits an invalid locator to the API.
While `chain` is always provided in the current `createWallet` call-path, the optional signature (`chain?: C`) allows future callers to inadvertently trigger this. The block should throw an explicit error when `chain` is missing rather than silently falling through:
```suggestion
if (signer.type === "server") {
if (chain == null) {
throw new WalletCreationError(
"chain is required to register a server signer"
);
}
const { derivedAddress } = deriveServerSignerDetails(
signer as unknown as ServerSignerConfig,
chain,
this.apiClient.projectId,
this.apiClient.environment
);
return { signer: `server:${derivedAddress}` };
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/wallets/src/wallets/types.ts
Line: 114-116
Comment:
**Unused type `DelegatedSignerInput`**
`DelegatedSignerInput` is defined here but is not referenced anywhere else in the codebase and is not exported from `packages/wallets/src/index.ts`. Per the project's convention of removing unused code, this type should be removed until it is actually needed.
**Rule Used:** Remove unused code from PRs to keep them lean. Add... ([source](https://app.greptile.com/review/custom-context?memory=83771aea-3c5f-4a37-9170-8ac881cd5efd))
**Learnt From**
[Paella-Labs/crossbit-main#20960](https://github.com/Paella-Labs/crossbit-main/pull/20960)
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/wallets/quickstart-devkit/components/activity.tsx
Line: 56
Comment:
**`token_symbol` is optional — missing fallback**
According to the `WalletsActivityResponseUnstableDTO` OpenAPI schema, `token_symbol` is **not** in the `required` array, meaning it can be absent for some activity events (e.g. unknown or non-standard tokens). The previous code guarded this with `transfer.token.symbol ?? transfer.token.locator`, but the new code renders nothing when `token_symbol` is undefined, leaving a blank next to the amount.
Consider falling back to a meaningful placeholder, for example:
```suggestion
{event.amount} {event.token_symbol ?? ""}
```
or a more informative fallback such as `event.token_symbol ?? event.mint_hash?.slice(0, 8) ?? "Unknown"`.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "fix: lint formatting..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (signer.type === "server" && chain != null) { | ||
| const { derivedAddress } = deriveServerSignerDetails( | ||
| signer as unknown as ServerSignerConfig, | ||
| chain, | ||
| this.apiClient.projectId, | ||
| this.apiClient.environment | ||
| ); | ||
| return { signer: `server:${derivedAddress}` }; | ||
| } | ||
| return { signer: getSignerLocator(signer) as string }; |
There was a problem hiding this comment.
When signer.type === "server" but chain is null / undefined, the condition on line 382 is false and execution falls through to getSignerLocator(signer). getSignerLocator has no special handling for "server", so it hits the catch-all return signer.type as SignerLocator and returns the bare string "server" instead of "server:<derivedAddress>". This silently submits an invalid locator to the API.
While chain is always provided in the current createWallet call-path, the optional signature (chain?: C) allows future callers to inadvertently trigger this. The block should throw an explicit error when chain is missing rather than silently falling through:
| if (signer.type === "server" && chain != null) { | |
| const { derivedAddress } = deriveServerSignerDetails( | |
| signer as unknown as ServerSignerConfig, | |
| chain, | |
| this.apiClient.projectId, | |
| this.apiClient.environment | |
| ); | |
| return { signer: `server:${derivedAddress}` }; | |
| } | |
| return { signer: getSignerLocator(signer) as string }; | |
| if (signer.type === "server") { | |
| if (chain == null) { | |
| throw new WalletCreationError( | |
| "chain is required to register a server signer" | |
| ); | |
| } | |
| const { derivedAddress } = deriveServerSignerDetails( | |
| signer as unknown as ServerSignerConfig, | |
| chain, | |
| this.apiClient.projectId, | |
| this.apiClient.environment | |
| ); | |
| return { signer: `server:${derivedAddress}` }; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/wallets/src/wallets/wallet-factory.ts
Line: 382-391
Comment:
**Silent fallthrough produces invalid server signer locator**
When `signer.type === "server"` but `chain` is `null` / `undefined`, the condition on line 382 is false and execution falls through to `getSignerLocator(signer)`. `getSignerLocator` has no special handling for `"server"`, so it hits the catch-all `return signer.type as SignerLocator` and returns the bare string `"server"` instead of `"server:<derivedAddress>"`. This silently submits an invalid locator to the API.
While `chain` is always provided in the current `createWallet` call-path, the optional signature (`chain?: C`) allows future callers to inadvertently trigger this. The block should throw an explicit error when `chain` is missing rather than silently falling through:
```suggestion
if (signer.type === "server") {
if (chain == null) {
throw new WalletCreationError(
"chain is required to register a server signer"
);
}
const { derivedAddress } = deriveServerSignerDetails(
signer as unknown as ServerSignerConfig,
chain,
this.apiClient.projectId,
this.apiClient.environment
);
return { signer: `server:${derivedAddress}` };
}
```
How can I resolve this? If you propose a fix, please make it concise.| export type DelegatedSignerInput = { | ||
| signer: string | ServerSignerConfig; | ||
| }; |
There was a problem hiding this comment.
DelegatedSignerInput
DelegatedSignerInput is defined here but is not referenced anywhere else in the codebase and is not exported from packages/wallets/src/index.ts. Per the project's convention of removing unused code, this type should be removed until it is actually needed.
Rule Used: Remove unused code from PRs to keep them lean. Add... (source)
Learnt From
Paella-Labs/crossbit-main#20960
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/wallets/src/wallets/types.ts
Line: 114-116
Comment:
**Unused type `DelegatedSignerInput`**
`DelegatedSignerInput` is defined here but is not referenced anywhere else in the codebase and is not exported from `packages/wallets/src/index.ts`. Per the project's convention of removing unused code, this type should be removed until it is actually needed.
**Rule Used:** Remove unused code from PRs to keep them lean. Add... ([source](https://app.greptile.com/review/custom-context?memory=83771aea-3c5f-4a37-9170-8ac881cd5efd))
**Learnt From**
[Paella-Labs/crossbit-main#20960](https://github.com/Paella-Labs/crossbit-main/pull/20960)
How can I resolve this? If you propose a fix, please make it concise.| <div> | ||
| <div className="font-medium"> | ||
| {transfer.token.amount} {transfer.token.symbol ?? transfer.token.locator} | ||
| {event.amount} {event.token_symbol} |
There was a problem hiding this comment.
token_symbol is optional — missing fallback
According to the WalletsActivityResponseUnstableDTO OpenAPI schema, token_symbol is not in the required array, meaning it can be absent for some activity events (e.g. unknown or non-standard tokens). The previous code guarded this with transfer.token.symbol ?? transfer.token.locator, but the new code renders nothing when token_symbol is undefined, leaving a blank next to the amount.
Consider falling back to a meaningful placeholder, for example:
| {event.amount} {event.token_symbol} | |
| {event.amount} {event.token_symbol ?? ""} |
or a more informative fallback such as event.token_symbol ?? event.mint_hash?.slice(0, 8) ?? "Unknown".
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/wallets/quickstart-devkit/components/activity.tsx
Line: 56
Comment:
**`token_symbol` is optional — missing fallback**
According to the `WalletsActivityResponseUnstableDTO` OpenAPI schema, `token_symbol` is **not** in the `required` array, meaning it can be absent for some activity events (e.g. unknown or non-standard tokens). The previous code guarded this with `transfer.token.symbol ?? transfer.token.locator`, but the new code renders nothing when `token_symbol` is undefined, leaving a blank next to the amount.
Consider falling back to a meaningful placeholder, for example:
```suggestion
{event.amount} {event.token_symbol ?? ""}
```
or a more informative fallback such as `event.token_symbol ?? event.mint_hash?.slice(0, 8) ?? "Unknown"`.
How can I resolve this? If you propose a fix, please make it concise.
🔥 Smoke Test Results✅ Status: Passed Statistics
✅ All smoke tests passed!All critical flows are working correctly. This is a non-blocking smoke test. Full regression tests run separately. |
4 participants
Description
Merges
mainintowallets-v1, resolving all merge conflicts. The primary conflict area was integrating main's server signer support into wallets-v1's refactored wallet API (which renamedsigner→recovery,delegatedSigners→signers, and splitgetOrCreateWalletintogetWallet/createWallet).Key conflict resolutions
wallet-factory.ts: Integrated server signer address derivation intocreateWallet,registerSigners, andvalidateSigners. Server signers are resolved toserver:${derivedAddress}format usingderiveServerSignerDetails.wallet.ts: AddedresolveSigner()helper to handlestring | ServerSignerConfig | undefinedsigner options, unifying device and server signer resolution. Also fixed error messages insubmitApprovalsthat were printing[object Object]instead ofpendingApproval.signer.locator.SignerLocatortype cast onlocator()return to satisfy theSignerinterface.signers/types.ts: AddedServerInternalSignerConfigto theInternalSignerConfigunion type.activity.tsx(quickstart-devkit): Fixed demo app to use.eventsinstead of.datato match the actualWalletsActivityResponseUnstableDtoshape from the merged openapi types. The wallets-v1 branch had introduced a.dataproperty that doesn't exist on this DTO.Items for careful review
as unknown as ServerSignerConfigcasts invalidateSignersandregisterSigners— these bypass type safety becauseSignerConfigForChain<C>doesn't directly include thesecretfield. Consider whetherServerSignerConfigshould be part ofSignerConfigForChain.compareSignerConfigsis skipped for server signers — the wallet API returns server signers as"external-wallet"type, so field-by-field comparison fails ontype. Locator matching is used instead. Verify this is sufficient validation.getWalletno longer validates server signer address mismatch — in wallets-v1,getWallettakesWalletArgsFor(norecovery), so the server signer address check from main doesn't apply. The corresponding test was updated to expect success instead of rejection. Confirm this is acceptable behavior.transfer.sender.address/transfer.recipient.address/transfer.token.amounttoevent.from_address/event.to_address/event.amount. These match the generatedWalletsActivityResponseUnstableDtotype but should be verified against actual API responses at runtime.Test plan
pnpm test:vitest— 225 passed, 68 skipped)pnpm build:libs)pnpm lint)registerSignersandvalidateSignerslogicPackage updates
Includes version bumps and changelogs from main for multiple packages (client-sdk-base, client-sdk-react-ui, server-sdk, wallets-sdk, etc.). Also includes
@basis-theory-ai/react→@basis-theory/react-agenticrename and@noble/hashesversion unification to 1.8.0 from main. No new changeset added for the merge conflict resolution itself — may need one if this counts as a functional change to@crossmint/wallets-sdk.Link to Devin session: https://crossmint.devinenterprise.com/sessions/33b6ec310a5b4bb59b292da3dae4c515
Requested by: @guilleasz-crossmint