Skip to content

feat(sdk-core): add wrap() and unwrap() to DefiVault - #9569

Merged
kamleshmugdiya merged 1 commit into
masterfrom
claude/defi-661-changes-16c33b
Sep 7, 2026
Merged

feat(sdk-core): add wrap() and unwrap() to DefiVault#9569
kamleshmugdiya merged 1 commit into
masterfrom
claude/defi-661-changes-16c33b

Conversation

@kamleshmugdiya

Copy link
Copy Markdown
Contributor

Implements DEFI-661wallet.defi.wrap() and wallet.defi.unwrap().

Both are thin orchestrators over a single wallet.sendMany, modelled on the existing withdrawFromVault. WP builds the WETH9 calldata and resolves the contract address from the vault binding; the SDK only forwards vaultId and amount.

const { txRequestId } = await wallet.defi.wrap({ vaultId, amount, walletPassphrase });
const { txRequestId } = await wallet.defi.unwrap({ vaultId, amount, walletPassphrase });

Changes

File Change
defi/iDefiVault.ts WrapOptions / WrapResult; wrap/unwrap on IDefiVault
defi/defiVault.ts wrap/unwrap sharing a private sendWrapIntent
wallet/wallet.ts wrapNative / unwrapNativewrap-native / unwrap-native intents
utils/mpcUtils.ts both sites — recipients exemption list and the EVM intent-shape switch
utils/tss/recipientUtils.ts NO_RECIPIENT_TX_TYPES, in both spellings
examples/ts/defi-vault-wrap.ts new, mirroring the existing defi-vault examples

Two deliberate choices worth flagging in review:

  • WrapResult.operationId is declared optional even though nothing populates it. No operation is minted for wrap/unwrap in v1, and sendWrapIntent deliberately does not call extractOperationId — it would only ever return undefined. Declaring the field now keeps M5's operation tracking from being a breaking change to a published SDK type.
  • vaultId is required. Binding the wrap to a vault is what supplies the per-enterprise authorization gate and the address-whitelist path server-side (TDD §3.6). M7 relaxes it to optional, which is backward-compatible.

defiParams is decoded with decodeWithCodec (the defiWithdraw pattern) rather than the bare as casts defiApprove/defiDeposit use, so an 18dp amount is validated as a numeric string and stays a string end-to-end.

Both spellings in NO_RECIPIENT_TX_TYPES

NO_RECIPIENT_TX_TYPES is matched against two different sources:

  • txParams.type — which is buildParams.type, the camelCase value passed to wallet.sendMany
  • txRequest.intent.intentTypekebab-case, as WP persists it

Signing paths that carry no txParamspendingApproval.approve()recreateTxRequest()signTxRequest() — only ever see the kebab-case spelling. Registering only one spelling leaves the other path throwing InvalidTransactionError before signing, so all four strings are registered and there is a regression test per path.

Pre-existing bug found, filed separately

That asymmetry is already live for the three existing DeFi types: defi-approve / defi-deposit / defi-withdraw are registered in camelCase only, so approving a pending approval for any of them throws today. Filed as DEFI-688 rather than folded in here — the fix also requires changing an existing assertion (recipientUtils.ts:67 asserts 'defi-deposit' must not be in the set) that deserves its own review. The new wrap/unwrap types are not affected.

public-types is intentionally not bumped

DEFI-661 asks to bump @bitgo/public-types to the version from DEFI-657 (6.60.0). Not done here, on purpose:

  • sdk-core imports no intent codec from public-types — only GetVaultResponse / VaultProtocol / MPC types. Intents are plain objects built by mpcUtils.populateIntent, so WrapNativeIntent / UnwrapNativeIntent are needed by wallet-platform, not by the SDK.
  • The ticket's stated baseline is stale: sdk-core is pinned at 6.58.0, not 6.56.0 (bumped in 867580cf71).
  • Repo convention bumps all seven dependent modules together with a yarn.lock consolidation, in its own build: commit.

wallet-platform still needs @bitgo/public-types@6.60.0 for DEFI-659.

Security note

Registering wrapNative in NO_RECIPIENT_TX_TYPES suppresses the SDK's missing-recipients guard, and verifyTssTransaction's client-side amount/destination comparison only runs when txParams.type === 'transfer' (abstractEthLikeNewCoins.ts:3218), so it never fires for DeFi types. After this change WP's server-side assertion is the only control on how much ETH a wrap moves. That is the accepted design (TDD §5.2); nothing here weakens it further.

Testing

  • modules/sdk-core642 passing, 1 pending (pre-existing). Adds 22 wrap/unwrap cases: sendMany shape (type + defiParams asserted exactly), txRequestId from both full and lite response shapes, operationId stays undefined, passphrase forwarded/omitted, missing vaultId/amount throwing with no network call, and defiParams codec validation.
  • modules/sdk-core/test/.../recipientUtils.ts — regression tests for camelCase, kebab-case, and txParams: undefined.
  • modules/bitgo/test/v2/unit/wallet.tspopulateIntent for both intents, proving the recipients assertion does not throw and the intent carries a plain amount rather than shareTokenAmount. Full file passes.
  • tsc --noEmit clean across sdk-core (src + test); eslint and prettier clean.

End-to-end only works once DEFI-659 is deployed; this is unit-tested against mocks.

Out of scope

No autoWrap / chaining into depositToVault (the UI drives that, M4), no confirmation polling, no operation tracking (M5).

TICKET: DEFI-661

🤖 Generated with Claude Code

@linear-code

linear-code Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

DEFI-661

@kamleshmugdiya
kamleshmugdiya marked this pull request as ready for review August 31, 2026 06:33
@kamleshmugdiya
kamleshmugdiya requested review from a team as code owners August 31, 2026 06:33
@kamleshmugdiya kamleshmugdiya self-assigned this Aug 31, 2026
Comment thread modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts
Comment thread modules/sdk-core/src/bitgo/defi/defiVault.ts
Comment thread modules/sdk-core/test/unit/bitgo/utils/tss/recipientUtils.ts
Comment thread modules/sdk-core/src/bitgo/defi/defiVault.ts Outdated
@kamleshmugdiya
kamleshmugdiya force-pushed the claude/defi-661-changes-16c33b branch from ee36234 to c6e5e3d Compare August 31, 2026 11:19
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

⚠️ Unit tests are failing on Node 26.x (Current release line, non-blocking). This is not an LTS version yet, so it does not block merge, but it signals an incompatibility to fix before Node 26.x becomes LTS.

View run

@kamleshmugdiya
kamleshmugdiya force-pushed the claude/defi-661-changes-16c33b branch 2 times, most recently from 89e2349 to 19690df Compare September 3, 2026 11:23
Expose ETH -> WETH wrap and WETH -> ETH unwrap on wallet.defi so the UI can
call them directly. Both are thin orchestrators over a single wallet.sendMany,
modelled on the existing withdrawFromVault; WP builds the WETH9 calldata and
resolves the contract address from the vault binding.

- iDefiVault: WrapOptions / WrapResult, plus wrap/unwrap on IDefiVault.
  operationId is declared optional now even though nothing populates it, so
  that adding operation tracking in M5 is not a breaking change to a published
  SDK type.
- defiVault: wrap/unwrap share sendWrapIntent, which deliberately does not call
  extractOperationId - no operation is minted in v1. sendWrapIntent validates
  the amount as a positive unsigned decimal integer string (zero, negative
  and hex forms are rejected since they forward straight into a value-moving
  WETH9 deposit()/withdraw() call), trims vaultId so whitespace-only values
  are rejected, and fails fast with a clear client-side error for non-EVM
  wallets (CoinFeature.EVM_COIN guard) instead of an opaque prebuild error.
- wallet: wrapNative / unwrapNative cases map to the wrap-native /
  unwrap-native intents, decoding defiParams with decodeWithCodec rather than
  a bare cast so an 18dp amount stays a string.
- mpcUtils: both sites - the recipients-required exemption list and the EVM
  intent-shape switch. Omitting the first makes the recipients assertion throw
  before the switch is reached. Resolved against master's ERC-7984
  wrapApprove/wrap intents by keeping both side by side.
- recipientUtils: register both the camelCase and kebab-case spellings.
  NO_RECIPIENT_TX_TYPES is matched against txParams.type (camelCase, from
  buildParams) and against intent.intentType (kebab-case, as WP persists it);
  signing paths that carry no txParams only ever see the latter.
- tests: boundary tests for amount/vaultId validation and the non-EVM guard
  in defiVault; an end-to-end regression test through signRequestBase ->
  resolveEffectiveTxParams -> verifyTransaction pinning the kebab-case
  no-recipients wiring for pendingApproval.approve() style signing paths.

@bitgo/public-types is intentionally left at 6.58.0: sdk-core imports no
intent codec from it, so the 6.60.0 publish carrying WrapNativeIntent /
UnwrapNativeIntent is only needed by wallet-platform.

DEFI-661
@kamleshmugdiya
kamleshmugdiya force-pushed the claude/defi-661-changes-16c33b branch from 19690df to 61ea5a0 Compare September 3, 2026 11:26
@kamleshmugdiya
kamleshmugdiya merged commit f866cea into master Sep 7, 2026
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants