Skip to content

feat: support multisig transfers - #15

Draft
cw35 wants to merge 13 commits into
masterfrom
feat/multisig-transfers
Draft

feat: support multisig transfers#15
cw35 wants to merge 13 commits into
masterfrom
feat/multisig-transfers

Conversation

@cw35

@cw35 cw35 commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Summary

  • support transfers from legacy and Safe multisig groups with separate --senders and --sender-threshold source flags
  • use --trace as create-or-join identity: create and sign a new transfer, or continue signing an existing one
  • add signature cancellation/unlock commands for both flows, plus legacy action-request cancellation
  • validate API responses and raw transactions before signing, and make concurrent creators converge on the same transfer

Commands

  • mixin-cli transfer ... --trace ... --senders ... --sender-threshold ...
  • mixin-cli transfer cancel ... (aliases: unlock, cancel-signature)
  • mixin-cli transfer cancel-request --request ...
  • mixin-cli safe transfer ... --trace ... --senders ... --sender-threshold ...
  • mixin-cli safe transfer cancel --trace ... (aliases: unlock, cancel-signature)

For legacy transfers, every signer supplies the same transfer fields because the legacy API has no lookup-by-trace endpoint. Existing Safe requests can be continued with --trace alone.

Test coverage

Added focused tests for deterministic signer ordering, exact-balance input selection, signed-output recovery, request and raw-transaction validation, malformed external UTXOs, duplicate/non-member signers, JSON receiver decoding, amount precision, and command registration.

Verification

  • go test ./...
  • go test -race -vet=off ./...
  • go vet ./cmd/safe ./cmd/transfer
  • go build plus CLI help smoke tests
  • git diff --check origin/master...HEAD

Self-review also fixed a legacy output-selection TOCTOU race and a Safe create/read race before this PR was opened.

Documentation

  • README.md: documented legacy and Safe multisig transfer, join-signing, signature cancellation, and legacy request cancellation workflows.

@cw35
cw35 marked this pull request as draft August 14, 2026 09:08
@cw35

cw35 commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up self-review fixes in 541b134:

  • bind legacy and Safe API raw transactions to the payload confirmed by the user before signing or broadcasting
  • validate request IDs, signer monotonicity, membership, thresholds, and signed responses
  • reject nil, non-positive, wrong-state, and inconsistent-member-order source outputs before SDK builders can panic
  • redact Safe views from every CLI JSON response
  • move legacy cancel confirmation before the sign-request side effect
  • avoid SDK single-response helpers that can panic on empty API arrays
  • reject invalid broadcast responses

Verification rerun:

  • go test -count=1 ./...
  • go test -race -vet=off -count=1 ./...
  • go vet ./cmd/safe ./cmd/transfer
  • go build ./...
  • transfer and Safe transfer help smoke tests
  • git diff --check

@lyricat
lyricat marked this pull request as ready for review August 18, 2026 01:22
@cw35
cw35 force-pushed the feat/multisig-transfers branch from 180ac6e to cedab34 Compare August 18, 2026 01:31
@mistermorph-reviewer

Copy link
Copy Markdown

Findings

[P2] Legacy 转账无法使用主网型 MIX... 接收地址

NormalizeMultisigDestination 会正常解码由主网地址成员构成的 MIX... 地址,但 legacy 多签路径随后无条件调用 mixin.NewMixAddress。该构造函数只接受 UUID 成员;主网成员必须使用 mixin.NewMainnetMixAddress。因此 README 新增的“--receivers MIX...”用法只对 UUID 型 MIX 地址有效,对同一解析器明确支持的主网型 MIX 地址会在创建交易前失败。这里应像 Safe 路径一样区分 UUID 成员和主网地址成员,或者在命令和文档中明确拒绝不支持的地址类型。

Example:使用 mixin.NewMainnetMixAddress([]string{XIN_ADDRESS_1, XIN_ADDRESS_2}, 1) 生成地址后执行 mixin-cli transfer ... --receivers MIX...。归一化会把参数展开为两个 XIN 地址并设置 threshold=1,随后 legacyTransferReceivermixin.NewMixAddress(members, threshold) 返回 invalid member ...,转账无法继续。相关位置:cmd/transfer/multisig.go:265

Review scope

  • 仓库:fox-one/mixin-cli,PR feat: support multisig transfers #15
  • 对比范围:GitHub master 的 PR base e89e2349b46d126ebef022b0e5af00899b55f290 到指定 head cedab34de7629eb7efb12e3bdb79a88651bdc5b9
  • 已读取 PR 描述及完整 diff:11 个文件,新增 3511 行、删除 76 行。
  • 重点检查了 legacy 与 Safe 多签转账的创建、续签、签名取消、请求取消、UTXO 分页与选择、外部响应和 raw transaction 校验,以及命令注册和 README 用法。

Validation

  • git diff --check e89e234...cedab34:通过。
  • go test -vet=off ./...:通过。
  • go vet ./cmd/safe ./cmd/transfer:通过。
  • go test ./...:未通过;失败来自本次 diff 之外的 cmd/sign/sign.go:71cmd/http/http.go:80,原因均为 cobra.Command.Println 参数末尾含冗余换行。相关变更包的测试本身通过。

Residual risks

  • 未连接真实 Mixin API 执行端到端多签转账;并发创建、服务端响应字段差异及广播行为仅通过代码、mock 测试和 SDK 类型检查验证。

@mistermorph-reviewer mistermorph-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

[P2] 校验按 trace 查到的 transfer 是否与本次请求一致 — cmd/transfer/multisig.go:410

ReadTransfer 成功后,代码只检查响应非 nil,便把当前 legacy multisig transfer 判为已完成;没有核对返回快照的 TraceIDAssetIDAmount、收款方或 memo。--trace 是用户可指定的,因此只要当前账户曾把同一 trace 用于另一笔 transfer,本命令就会打印 transaction already completed 并直接成功退出,即使本次资产、金额、来源 multisig 或目标完全不同。这样会把未执行的转账报告为已完成。应在返回成功前,把快照中可用的交易身份字段与 input、sender group 和 receiver 做一致性校验;不一致时应明确报错,而不是按完成处理。

已验证 Example: 新增的 TestResolveLegacyMultisigTransactionSkipsOutputsWhenTraceExists 传入 TransferInput{AssetID: "asset", Amount: 1, TraceID: "trace"},但 mock 仅返回 Snapshot{TraceID: "trace"},其 AssetID 为空、Amount 为 0,也没有任何收款方信息;resolveLegacyMultisigTransaction 仍返回 UTXOStateSpent。这直接验证了不匹配或信息不足的响应会被接受为已完成。

Review scope

仅审查 cedab34de7629eb7efb12e3bdb79a88651bdc5b9...50b52113e2a7c370d15b1c0bc48507aa335cab03 的增量,共涉及 README.mdcmd/transfer/multisig.gocmd/transfer/multisig_test.go。同时核对了 transfer 命令入口、legacy multisig 的 output 查找与交易匹配逻辑,以及依赖 SDK 中 ReadTransferSnapshot 和错误码判断的实现。

Validation

  • git diff --check cedab34de7629eb7efb12e3bdb79a88651bdc5b9...50b52113e2a7c370d15b1c0bc48507aa335cab03:通过。
  • go test ./cmd/transfer:通过。
  • go vet ./cmd/transfer:通过。

Residual risks

未连接真实 Mixin API 验证已完成 legacy multisig transfer 的快照字段形态;本次判断基于仓库锁定的 SDK 实现、类型定义和单元测试行为。

@cw35
cw35 marked this pull request as draft August 18, 2026 12:36
@cw35

cw35 commented Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Review notes follow-up

  • Legacy mainnet MIX... receiver — fixed in 08cf3f2. legacyTransferReceiver now selects NewMainnetMixAddress for mainnet members, matching the Safe path. Added TestLegacyTransferReceiverAcceptsMainnetMembers to cover the encoded-mainnet-address flow.
  • Legacy ReadTransfer(trace) identity issue — already fixed in fcff2f4. The current implementation no longer uses ReadTransfer or snapshot trace matching. It resolves pending transfers from signed outputs plus the canonical MultisigRequest, checks spent outputs for completed-transfer idempotency, and only then falls back to unspent outputs.

Verification after the latest fix:

  • go test ./...
  • go test -race ./cmd/transfer
  • go vet ./...
  • go build ./...
  • git diff --check

All 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.

1 participant