feat: accept all intent message types in SubmitIntent#101
Conversation
There was a problem hiding this comment.
Code Review — feat: accept all intent message types in SubmitIntent
Verdict: APPROVE ✅
Clean, well-structured PR. The change generalizes SubmitIntent from RegisterMessage-only to all six arkd intent message types. This is a necessary evolution — contract VTXOs need to authenticate delete, get-pending-tx, estimate-fee, get-intent, and get-data operations, not just registration.
What I verified
1. Type safety & interface correctness
- Confirmed all six arkd types (
RegisterMessage,EstimateIntentFeeMessage,DeleteMessage,GetPendingTxMessage,GetIntentMessage,GetDataMessage) implementEncode() (string, error)andDecode(string) errorin the upstreamarkd/pkg/ark-lib/intentpackage. TheIntentMessageinterface (service.go:28-31) is correctly satisfied by all six.
2. Wire compatibility preserved
- The proto (
service.proto) changes are comment-only — no wire format change. TheIntentmessage still carriesproofandmessageas strings. Existing clients sendingregistermessages continue to work identically. - The public client package (
pkg/client/transport_client.go) uses rawstringfields — completely unaffected.
3. Validation logic is correct
validateMessage()(intent.go:80-99) correctly handles the asymmetry:RegisterMessageandEstimateIntentFeeMessagecarry bothValidAt+ExpireAt; the other four only carryExpireAt. The type switch extracts the right fields for each.- The
defaultbranch properly rejects unknown types. - Zero-value timestamps are treated as "no bound" — consistent with the original behavior.
4. Parser logic is sound
parseIntent()(parser.go:17-66) does a two-phase decode: peek atBaseMessage.Typeviajson.Unmarshal, then dispatch to the concrete struct'sDecode(). This is the standard pattern in arkd and avoids any type confusion.- Error messages include the message type for debuggability (
fmt.Errorf("invalid %s message: %w", base.Type, err)).
5. No cross-repo breakage
SubmitFinalization(finalization.go) only usesIntent.Proof.Packet— never touchesIntent.Messageat all. The type change fromintent.RegisterMessagetoIntentMessageis invisible to finalization.- Integration tests (
test/*.go) use the client package (pkg/client) which operates on raw strings — they compile and behave identically. - TypeScript SDK already supports
DeleteMessageandGetPendingTxMessage— this aligns the emulator with what the SDK already sends.
6. Test coverage is adequate
intent_test.go: covers all six types valid, zero timestamps, expired, not-valid-yet, and unsupported type. Good.parser_test.go: covers all six types, missing proof, missing message, invalid proof, unsupported type. Good.- The
unknownMessagetest stub inintent_test.go:58-61correctly exercises the default branch.
Minor observations (non-blocking)
-
parser.go:37—json.Unmarshal([]byte(message), &base): The message string is already JSON (produced byEncode()), so the cast to[]byteis fine, but worth noting this assumesEncode()always returns valid JSON. This is guaranteed by the arkd implementation but isn't validated here. Not a real risk — just a trust boundary note. -
Time-of-check / time-of-use on expiry:
validateMessage()checksExpireAtat parse time, but the intent proof may be used slightly later. The original code had the same gap. Not introduced by this PR.
No protocol-critical concerns. The VTXO signing path (SubmitIntent body, lines 20-75) is completely unchanged — only the validation preamble was generalized. Forfeit and commitment signing in SubmitFinalization are untouched.
🤖 Reviewed by Arkana
please review @louisinger |
fixes #34
SubmitIntentonly decoded the register intent message, so contract vtxos could register but never authenticate any other intent based arkd operation. this generalizes the path to accept all six arkd intent message types, keeping the existing register flow identical and wire compatible while adding tests for decoding and validation.