feat(wallets): add deployImmediately flag to addSigner with default true#1924
feat(wallets): add deployImmediately flag to addSigner with default true#1924maxsch-xmint wants to merge 6 commits into
Conversation
Co-Authored-By: max@paella.dev <max@paella.dev>
Co-Authored-By: max@paella.dev <max@paella.dev>
Original prompt from Max
|
🤖 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:
|
🦋 Changeset detectedLatest commit: 2a1e3a3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/wallets/src/wallets/wallet.test.ts:732-755
**Missing test: `prepareOnly: true` with new EVM transaction flow**
The only EVM `prepareOnly` test was updated to add `deployImmediately: false` to preserve the old signature path. There is no test covering the new default combination — `prepareOnly: true` + `deployImmediately: true` for EVM — which would call the `transaction` branch of `completeSignerRegistration` and return `{ transactionId }` instead of `{ signatureId }`. Callers currently relying on `prepareOnly` for EVM will receive a different shape in their result object once they upgrade, and this path has no dedicated unit coverage.
Reviews (1): Last reviewed commit: "chore: add changeset for deployImmediate..." | Re-trigger Greptile |
Co-Authored-By: max@paella.dev <max@paella.dev>
|
Good catch, scaly friend 🐍 — added a dedicated test for |
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/wallets/src/wallets/types.ts:59-61
**`AddSignerReturnType` missing `transactionId` for EVM**
The EVM branch of `AddSignerReturnType` only includes `signatureId?: string`, but with the new `deployImmediately: true` default the runtime response contains `transactionId` (not `signatureId`). Any EVM caller using `prepareOnly: true` with the new default will receive a `transactionId` at runtime that TypeScript doesn't know about — accessing `result.transactionId` would be a type error. The test at line 750 of `wallet.test.ts` already demonstrates this gap by expecting `result.transactionId` on an `evmWallet`, which has no such field in the current type definition.
The EVM variant should be widened to `Signer & { signatureId?: string; transactionId?: string }` to cover both the legacy signature flow and the new on-chain transaction flow.
### Issue 2 of 2
packages/wallets/src/wallets/wallet.ts:806-807
**EVM transaction response yields incorrect `status: "success"` before on-chain confirmation**
When the API returns a `transaction` for an EVM wallet (the new `deployImmediately: true` default), the code at line 806 correctly captures the `pendingOperation`, but `registeredSigner` was already set via `mapApiSignerToSigner` a few lines earlier. `mapApiSignerToSigner` has no branch for EVM + `transaction` response: it falls through to the comment *"If chains field is empty, the signer was created during wallet creation"* and returns `status: "success"`. For the `prepareOnly: true` path this means the caller receives `{ status: "success", transactionId: "..." }` — reporting success before the transaction is confirmed. By contrast, Solana/Stellar correctly propagates `transaction.status` (e.g. `"pending"`) through `mapApiSignerToSigner`.
`mapApiSignerToSigner` should be updated to read `transaction.status` for EVM responses that include a `transaction` field, mirroring the existing Solana/Stellar logic.
Reviews (2): Last reviewed commit: "test: add prepareOnly + deployImmediatel..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if ("transaction" in response && response.transaction != null) { | ||
| pendingOperation = { type: "transaction", id: response.transaction.id }; |
There was a problem hiding this comment.
status: "success" before on-chain confirmation
When the API returns a transaction for an EVM wallet (the new deployImmediately: true default), the code at line 806 correctly captures the pendingOperation, but registeredSigner was already set via mapApiSignerToSigner a few lines earlier. mapApiSignerToSigner has no branch for EVM + transaction response: it falls through to the comment "If chains field is empty, the signer was created during wallet creation" and returns status: "success". For the prepareOnly: true path this means the caller receives { status: "success", transactionId: "..." } — reporting success before the transaction is confirmed. By contrast, Solana/Stellar correctly propagates transaction.status (e.g. "pending") through mapApiSignerToSigner.
mapApiSignerToSigner should be updated to read transaction.status for EVM responses that include a transaction field, mirroring the existing Solana/Stellar logic.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/wallets/src/wallets/wallet.ts
Line: 806-807
Comment:
**EVM transaction response yields incorrect `status: "success"` before on-chain confirmation**
When the API returns a `transaction` for an EVM wallet (the new `deployImmediately: true` default), the code at line 806 correctly captures the `pendingOperation`, but `registeredSigner` was already set via `mapApiSignerToSigner` a few lines earlier. `mapApiSignerToSigner` has no branch for EVM + `transaction` response: it falls through to the comment *"If chains field is empty, the signer was created during wallet creation"* and returns `status: "success"`. For the `prepareOnly: true` path this means the caller receives `{ status: "success", transactionId: "..." }` — reporting success before the transaction is confirmed. By contrast, Solana/Stellar correctly propagates `transaction.status` (e.g. `"pending"`) through `mapApiSignerToSigner`.
`mapApiSignerToSigner` should be updated to read `transaction.status` for EVM responses that include a `transaction` field, mirroring the existing Solana/Stellar logic.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Both valid catches, cold-blooded one 🦎 — fixed in 725c411:\n\n1. AddSignerReturnType for EVM now includes transactionId?: string alongside signatureId?: string\n2. mapApiSignerToSigner now checks for transaction field on EVM responses and propagates transaction.status (e.g. \"pending\") instead of falling through to \"success\"\n\nTest updated to assert status: \"pending\" on the prepareOnly + transaction path.
…action status Co-Authored-By: max@paella.dev <max@paella.dev>
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/wallets/src/wallets/wallet.ts:806-826
**Solana/Stellar `chains`-response guard removed**
The old code explicitly threw `"Expected transaction in response for Solana/Stellar chain"` when the API returned a `chains` field for a Solana or Stellar wallet (e.g. `chains: {}`). The new response-driven logic removes that guard: if a Solana/Stellar response includes a `chains` field but no `transaction`, the branch at line 808 (`else if ("chains" in response)`) now executes instead of erroring. For Solana/Stellar, `getPendingSignerOperation` always returns `null` from a `chains`-only path, so the SDK would silently report `status: "success"` — with a `transactionId` field missing at runtime even though the `AddSignerReturnType<"solana">` type declares it as `string`. The corresponding error-detection test was removed rather than updated, so the gap is untested.
Reviews (3): Last reviewed commit: "fix: add transactionId to EVM AddSignerR..." | Re-trigger Greptile |
…andling Co-Authored-By: max@paella.dev <max@paella.dev>
|
Fair point, you slippery lizard 🐊 — restored the Solana/Stellar guard. The logic now is:\n\n1. If response has |
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/wallets/src/utils/signer-mapping.ts:98-121
**Retry/idempotency broken for EVM `deployImmediately` transactions**
`getPendingSignerOperation` has no branch for EVM + `transaction` field. `mapApiSignerToSigner` was correctly updated to propagate `transaction.status` for EVM, but `getPendingSignerOperation` still only checks `chains` for EVM. When `getSigner` returns an EVM signer whose `transaction` is still `"pending"` (the now-default `deployImmediately: true` flow), `getSignerState` returns `pendingOperation: null`. Back in `addSigner`, neither guard fires (`isApprovedSignerStatus("pending") === false`, `pendingOperation == null`), so the code falls through to a fresh `registerSigner` call instead of resuming the in-flight transaction. The fix is to make `getPendingSignerOperation` response-driven — check for `transaction` first regardless of chain — mirroring what was already done in `mapApiSignerToSigner`.
Reviews (4): Last reviewed commit: "fix: restore Solana/Stellar transaction ..." | Re-trigger Greptile |
…on flow Co-Authored-By: max@paella.dev <max@paella.dev>
|
Nice find, you cold-blooded crawler 🐸 — |
|
Reviews (5): Last reviewed commit: "fix: make getPendingSignerOperation resp..." | Re-trigger Greptile |
1 participant
Description
Adds a
deployImmediatelyoption towallet.addSigner()for EVM wallets. Whentrue(the new default), the signer is registered on-chain immediately via a transaction — matching how Solana/Stellar already behave — instead of the previous lazy typed-data signature-request flow.This is the SDK counterpart to Paella-Labs/crossbit-main#26626, which adds the backend support for this flag.
Changes:
RegisterSignerParamsgains optionaldeployImmediately?: booleanAddSignerOptionsgains optionaldeployImmediately?: boolean(defaults totruefor EVM)AddSignerReturnTypefor EVM widened to includetransactionId?: stringaddSigneris now response-driven: checks fortransactionfirst, preserves Solana/Stellar guard, then falls back tochainsmapApiSignerToSignerpropagatestransaction.statusfor EVM (no false "success" before confirmation)getPendingSignerOperationis response-driven: checkstransactionfirst (any chain) for correct retry/idempotencyTest plan
wallet.test.ts):deployImmediately: trueis passed by default for EVM add-signer callsdeployImmediately: falsecan be explicitly setprepareOnly: true+deployImmediately: truereturns{ transactionId, status: "pending" }prepareOnly: true+deployImmediately: falsereturns{ signatureId }transactionandchainsthrowsPackage updates
@crossmint/wallets-sdk: minor (changeset added)Link to Devin session: https://crossmint.devinenterprise.com/sessions/8e002024ca984142a4f351c4d9adfb18
Requested by: @maxsch-xmint