feat(wallets): add deployImmediately flag to EVM addSigner with default true#1925
feat(wallets): add deployImmediately flag to EVM addSigner with default true#1925maxsch-xmint wants to merge 4 commits into
Conversation
…lt true 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: b4b7030 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.ts:777-778
Non-EVM chains still forward an explicit `deployImmediately: false` to the API. The PR states Solana/Stellar are unaffected, and the default path (`options` not set) correctly omits the field for those chains. However, when a caller passes `{ deployImmediately: false }` on a Solana/Stellar wallet, `options?.deployImmediately` resolves to `false`, `false ?? undefined` stays `false`, and `false != null` is truthy — so `deployImmediately: false` is included in the request body. If the backend validates this field on non-EVM signers it will either error or silently diverge from the stated behaviour.
```suggestion
const isEvm = this.chain !== "solana" && this.chain !== "stellar";
const deployImmediately = isEvm ? (options?.deployImmediately ?? true) : undefined;
```
Reviews (1): Last reviewed commit: "chore: add changeset for deployImmediate..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const isEvm = this.chain !== "solana" && this.chain !== "stellar"; | ||
| const deployImmediately = options?.deployImmediately ?? (isEvm ? true : undefined); |
There was a problem hiding this comment.
deployImmediately: false to the API. The PR states Solana/Stellar are unaffected, and the default path (options not set) correctly omits the field for those chains. However, when a caller passes { deployImmediately: false } on a Solana/Stellar wallet, options?.deployImmediately resolves to false, false ?? undefined stays false, and false != null is truthy — so deployImmediately: false is included in the request body. If the backend validates this field on non-EVM signers it will either error or silently diverge from the stated behaviour.
| const isEvm = this.chain !== "solana" && this.chain !== "stellar"; | |
| const deployImmediately = options?.deployImmediately ?? (isEvm ? true : undefined); | |
| const isEvm = this.chain !== "solana" && this.chain !== "stellar"; | |
| const deployImmediately = isEvm ? (options?.deployImmediately ?? true) : undefined; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/wallets/src/wallets/wallet.ts
Line: 777-778
Comment:
Non-EVM chains still forward an explicit `deployImmediately: false` to the API. The PR states Solana/Stellar are unaffected, and the default path (`options` not set) correctly omits the field for those chains. However, when a caller passes `{ deployImmediately: false }` on a Solana/Stellar wallet, `options?.deployImmediately` resolves to `false`, `false ?? undefined` stays `false`, and `false != null` is truthy — so `deployImmediately: false` is included in the request body. If the backend validates this field on non-EVM signers it will either error or silently diverge from the stated behaviour.
```suggestion
const isEvm = this.chain !== "solana" && this.chain !== "stellar";
const deployImmediately = isEvm ? (options?.deployImmediately ?? true) : undefined;
```
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Good catch from the cold-blooded code reviewer 🦎 — applied the suggested fix in 163b381. Non-EVM chains now always get undefined regardless of what the caller passes.
Co-Authored-By: max@paella.dev <max@paella.dev>
Co-Authored-By: max@paella.dev <max@paella.dev>
|
Reviews (2): Last reviewed commit: "style: remove redundant parentheses for ..." | Re-trigger Greptile |
1 participant
Description
Adds
deployImmediatelyoption toaddSigner()for EVM wallets, matching the new backend flag from crossbit-main#26626. Whentrue, the API returns an on-chain registration transaction (installed immediately) instead of the default lazy signature-request flow.SDK defaults to
truefor EVM chains — signer registration now goes on-chain immediately by default. Callers can opt out with{ deployImmediately: false }. Non-EVM chains (Solana/Stellar) are unaffected.Changes:
RegisterSignerParams.deployImmediately?: boolean— new API paramAddSignerOptions.deployImmediately?: boolean— new user-facing optionwallet.addSigner()sendsdeployImmediately: trueby default for EVM, omits it for Solana/StellargetPendingSignerOperationdetects transaction chain entries (viaonChainfield) and returnstype: "transaction"so the SDK approves the install tx instead of a signatureAddSignerReturnTypefor EVM gains optionaltransactionId(returned inprepareOnlymode for the transaction flow)Test plan
signer-mapping.test.ts: new test for EVMdeployImmediatelychain entry withonChainfield returningtype: "transaction"wallet.test.ts:deployImmediately: trueis sent by default for EVMdeployImmediately: falseoverride is forwardedonChainfieldprepareOnlyreturnstransactionIdfor the transaction flowPackage updates
@crossmint/wallets-sdk: minor (changeset added)Link to Devin session: https://crossmint.devinenterprise.com/sessions/1ca455c73e85421ca99667fda69b0cee
Requested by: @maxsch-xmint