refactor: move DAppMetadata fields into CustomMessagePayload#85
refactor: move DAppMetadata fields into CustomMessagePayload#85aminsato wants to merge 8 commits into
Conversation
Adds two optional string fields to CustomMessagePayload so the originating dApp's URL and human-readable name flow through the extension → relay → mobile signer path. Without them, mobile clients had no proto-level place to read dApp identity from for the post-keysign Overview screen. Both fields are optional, so older producers and consumers keep working unchanged. Favicon is derived on the consumer side from origin, so no separate icon_url is needed. Closes #84 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (7)
💤 Files with no reviewable changes (3)
WalkthroughThe PR updates the dApp context schema in CustomMessagePayload by adding ChangesdApp Context Schema Migration
Repository Configuration Cleanup
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
aminsato
requested review from
Ehsan-saradar,
gastonm5,
johnnyluo,
rcoderdev,
realpaaao and
rkokhatskyi
Update protobuf definitions and regenerate Swift/Go bindings to move dapp_metadata out of KeysignPayload into KeysignMessage. Also remove origin and dapp_name fields from CustomMessagePayload. Adjusts generated decoding/encoding, equality checks and field storage in Sources/swift/.../*.pb.swift and go/.../*.pb.go to match the new proto layout.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@proto/vultisig/keysign/v1/keysign_message.proto`:
- Line 28: The proto introduced a DAppMetadata field (dapp_metadata) that
reintroduces icon_url and diverges from the intended origin/name-only contract;
replace usage of the DAppMetadata field in the KeysignMessage with explicit
optional string fields for the dApp origin and dApp name (e.g., optional string
origin and optional string dapp_name) and remove/stop referencing the
DAppMetadata message (and its icon_url) so the wire schema matches the
origin/name-only contract expected by consumers; update any references to
keysign_message.dapp_metadata to use keysign_message.origin and
keysign_message.dapp_name instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 864ed4cb-ce4c-4dbc-a843-bdabdff8f6f8
⛔ Files ignored due to path filters (1)
go/vultisig/keysign/v1/keysign_message.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (2)
Sources/swift/vultisig/keysign/v1/keysign_message.pb.swiftproto/vultisig/keysign/v1/keysign_message.proto
aminsato
changed the title
Update DAppMetadata field number from 50 to 10 in the proto and regenerate protobuf outputs. Files changed: proto/vultisig/keysign/v1/keysign_message.proto and the generated Go and Swift sources (go/vultisig/keysign/v1/keysign_message.pb.go, Sources/swift/vultisig/keysign/v1/keysign_message.pb.swift). Adjusts field tags, raw descriptor bytes, and encode/decode branches to use field number 10. Note: this modifies the on-the-wire tag for dapp_metadata and is not backward-compatible with messages using the previous tag (50); update producers/consumers accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@proto/vultisig/keysign/v1/keysign_message.proto`:
- Line 28: The dapp_metadata field was retagged to 10 causing an extra
wire-compatibility break; restore its original tag by changing the declaration
of the dapp_metadata field in keysign_message.proto (the optional DAppMetadata
dapp_metadata field) back to use field number 50 instead of 10 so the message
keeps the prior wire layout and avoids an additional compatibility vector.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 559f1791-1479-4a14-b940-de7854a79e0f
⛔ Files ignored due to path filters (1)
go/vultisig/keysign/v1/keysign_message.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (2)
Sources/swift/vultisig/keysign/v1/keysign_message.pb.swiftproto/vultisig/keysign/v1/keysign_message.proto
| string payload_id = 7; | ||
| optional CustomMessagePayload custom_message_payload = 8; | ||
| optional string custom_payload_id = 9; | ||
| optional DAppMetadata dapp_metadata = 10; |
There was a problem hiding this comment.
Instead of moving this out of KeysignPayload , you can just add it to CustomMessagePayload , that way it won't cause incompatible issues
There was a problem hiding this comment.
🧹 Nitpick comments (1)
proto/vultisig/keysign/v1/custom_message_payload.proto (1)
14-15: ⚡ Quick winDocument canonical semantics for
originanddapp_namein-schema.Line 14 and Line 15 add cross-platform fields without format guidance. A short proto comment (e.g., expected URL form for
origin, display intent fordapp_name) helps prevent client-side interpretation drift.Suggested diff
message CustomMessagePayload { string method = 1; string message = 2; string vault_public_key_ecdsa = 3; string vault_local_party_id = 4; optional string chain = 5; + // Canonical dApp origin, preferably absolute HTTPS origin (scheme + host [+ port]). optional string origin = 6; + // Human-readable dApp display name shown in signing UX. optional string dapp_name = 7; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@proto/vultisig/keysign/v1/custom_message_payload.proto` around lines 14 - 15, The proto fields origin and dapp_name lack in-schema documentation; add brief proto comments above the fields in custom_message_payload.proto describing canonical semantics: for origin specify expected format (e.g., an origin URL including scheme and host like "https://example.com" or an origin string per RFC, and whether trailing slash or port allowed), and for dapp_name state it is a human-readable display name (not an identifier) for UI only, max length/encoding expectations, and whether clients must treat it as untrusted/display-only. Update the comments immediately above the optional string origin = 6; and optional string dapp_name = 7; declarations to reflect these guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@proto/vultisig/keysign/v1/custom_message_payload.proto`:
- Around line 14-15: The proto fields origin and dapp_name lack in-schema
documentation; add brief proto comments above the fields in
custom_message_payload.proto describing canonical semantics: for origin specify
expected format (e.g., an origin URL including scheme and host like
"https://example.com" or an origin string per RFC, and whether trailing slash or
port allowed), and for dapp_name state it is a human-readable display name (not
an identifier) for UI only, max length/encoding expectations, and whether
clients must treat it as untrusted/display-only. Update the comments immediately
above the optional string origin = 6; and optional string dapp_name = 7;
declarations to reflect these guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1e07658-a16e-416a-9872-b9f162f48da2
⛔ Files ignored due to path filters (1)
go/vultisig/keysign/v1/custom_message_payload.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (2)
Sources/swift/vultisig/keysign/v1/custom_message_payload.pb.swiftproto/vultisig/keysign/v1/custom_message_payload.proto
✅ Files skipped from review due to trivial changes (1)
- Sources/swift/vultisig/keysign/v1/custom_message_payload.pb.swift
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.idea/inspectionProfiles/Project_Default.xml:
- Around line 1-1097: The PR incorrectly includes an IDE config XML (.idea
inspection profile) — the <component name="InspectionProjectProfileManager"> /
<profile> export is unrelated to the protobuf/Swift changes; remove this file
from the commit, update the repo ignore rules (ensure .idea/ is listed in
.gitignore), and re-add only the intended proto and generated Swift files for
the CustomMessagePayload changes so the diff shows the actual symbols (e.g.,
origin, dapp_name additions in the proto and regenerated Swift outputs).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f57f27df-0d69-4e14-8db8-d69e5784c38f
⛔ Files ignored due to path filters (1)
go/vultisig/keysign/v1/custom_message_payload.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
.idea/caches/deviceStreaming.xml.idea/inspectionProfiles/Project_Default.xml.idea/misc.xmlSources/swift/vultisig/keysign/v1/custom_message_payload.pb.swiftproto/vultisig/keysign/v1/custom_message_payload.proto
💤 Files with no reviewable changes (1)
- .idea/misc.xml
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
aminsato
force-pushed
the
aminsato/84_add_dapp_origin_fields
branch
from
a138b26 to
05c8462
Compare
aminsato
changed the title
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nMessage Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
aminsato
changed the title
aminsato
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
aminsato
changed the title
| SignTon sign_ton = 42; | ||
| SignBitcoin sign_bitcoin = 43; | ||
| } | ||
| optional DAppMetadata dapp_metadata = 50; |
There was a problem hiding this comment.
can you please explain why we remove it from here?
There was a problem hiding this comment.
We removed DAppMetadata because its contents were moved into CustomMessagePayload (as you suggested in #85 (comment)). The three fields (origin, dapp_name, icon_url) now live directly on CustomMessagePayload at field numbers 6/7/8, so the standalone DAppMetadata message and the dapp_metadata field on KeysignPayload are no longer needed. Since DAppMetadata was never released to consumers, removing it is wire-safe.
There was a problem hiding this comment.
can you please don't check in files under .idea folder
There was a problem hiding this comment.
These were added by mistake earlier — Claude corrected it by removing the .gitignore inside the .idea/ folder, so the .idea/ files are no longer tracked on this branch.
Worth noting: the .idea/ folder shouldn't be in the repo at all — main currently has .idea/.gitignore and .idea/misc.xml checked in incorrectly, which is what this PR also cleans up.
gomesalexandre
left a comment
gomesalexandre
left a comment
There was a problem hiding this comment.
PR #85 — refactor: move DAppMetadata fields into CustomMessagePayload
This PR removes DAppMetadata (standalone message + dapp_metadata = 50 on KeysignPayload) and inlines the fields into CustomMessagePayload. The motivation is solid — dApp identity belongs alongside the custom message being signed, not scattered as a separate sub-message on the top-level payload. BUT there is a material wire-compat issue.
blocker: DAppMetadata and dapp_metadata = 50 are already on main
blocker: DAppMetadata was merged to main in a prior commit ("feat: add DAppMetadata to KeysignPayload"). The field optional DAppMetadata dapp_metadata = 50 is live on the default branch right now. This PR removes it entirely - including deleting the DAppMetadata message type and the field number 50 on KeysignPayload.
Even if no tagged release exists, consumers pinning the main branch (e.g. iOS / Android via buf.build registry or direct Go module pins) can already have this field in their generated code. A producer that populates dapp_metadata talking to a consumer that has merged this PR will silently drop the field - it becomes an unknown field on the receiver. Worse, if any downstream has started reading dapp_metadata from KeysignPayload, those reads now silently zero-out.
The PR description says "DAppMetadata was never released to consumers" - please verify that claim by confirming:
- No downstream repo (vultisig-ios, vultisig-android, vultisig-extension, vultisig-sdk) has merged or is running code that reads
KeysignPayload.dapp_metadata - No
buf.buildartifact was pushed with the old schema
If those are confirmed safe, add a note to the PR. If uncertain, the safe path is to keep field 50 as a reserved number: reserved 50; reserved "dapp_metadata"; - this prevents the number from being accidentally reused while still letting you remove the type.
field numbers on CustomMessagePayload - OK
praise: New fields 6 (origin), 7 (dapp_name), 8 (icon_url) are clean appends past the existing 5 fields. Numbers verified in both the .proto source and the generated rawDesc blobs in Go + Swift. No collision.
type inconsistency: icon_url vs origin / dapp_name
should-fix: origin and dapp_name are optional string (generates has-field sentinel, nil pointer in Go, _origin: String? in Swift). icon_url is a plain string (no optional, defaults to ""). If you're treating all three as "optional for compat", they should all be optional. An absent icon_url from an old sender will deserialize as "" which is fine for display, but it's architecturally inconsistent with the other two. Either make all three optional or document why icon_url deliberately isn't.
naming: origin vs url
q: The old DAppMetadata.url becomes origin here, with the doc comment defining it as "URL with scheme and host". That's a semantic narrowing (not just any URL, specifically an origin). The rename is good but worth confirming all existing producers (extension, SDK) that used to set dapp_metadata.url will update to custom_message_payload.origin and use the stricter format.
.idea/ cleanup
suggestion: The diff also removes .idea/misc.xml / .idea/.gitignore and adds .idea/ to .gitignore. Fine cleanup, but it's mixed into a proto refactor commit. Ideally a separate commit or at least acknowledged in the PR. Not blocking.
CI
Build passes, CodeRabbit skipped review. The DAppMetadata removal risk won't be caught by buf breaking if the baseline is main and main already has it - the breaking change is in the deletion direction.
Verdict: REQUEST_CHANGES - the dapp_metadata = 50 removal from a field already on main needs to be explicitly verified as safe before merge. Reserve the field number at minimum.
3 participants
Summary
DAppMetadatamessage (and theoptional DAppMetadata dapp_metadata = 50field onKeysignPayload) and inlines its three fields directly onCustomMessagePayload, where the dApp identity belongs alongside the message being signed:DAppMetadata.url→CustomMessagePayload.origin(optional string= 6)DAppMetadata.name→CustomMessagePayload.dapp_name(optional string= 7)DAppMetadata.icon_url→CustomMessagePayload.icon_url(string= 8)custom_message_payloadandcustom_payload_idfields onKeysignMessageto clarify their role in the dApp custom-signing flow (and the relay-stored payload reference).go/vultisig/keysign/v1/custom_message_payload.pb.go,keysign_message.pb.go) and Swift (Sources/swift/vultisig/keysign/v1/custom_message_payload.pb.swift,keysign_message.pb.swift) bindings.Wire compatibility
DAppMetadatawas never released to consumers, so removing it and its field onKeysignPayloadis safe.CustomMessagePayloadare additive and backward-compatible.Test plan
makeregenerates Go + Swift outputs with no other diffsbuf lintpassesorigin/dapp_name/icon_urlonCustomMessagePayloadCustomMessagePayloadproducers that don't set the new fields continue to deserialize correctly🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor