Skip to content

wallet: DEX order RPC methods (create/conclude/fill/freeze/list) - #2

Merged
erubboli merged 2 commits into
masterfrom
feat/order-rpc-methods
Sep 14, 2026
Merged

wallet: DEX order RPC methods (create/conclude/fill/freeze/list)#2
erubboli merged 2 commits into
masterfrom
feat/order-rpc-methods

Conversation

@erubboli

Copy link
Copy Markdown
Member

Adds the six order RPC wrappers the market-maker needs:

  • CreateOrder / ConcludeOrder / FillOrder / FreezeOrder
  • ListOwnOrders / ListAllActiveOrders (currency filters)
  • OutputValue (Coin|Token tagged wire type), OwnOrder/ActiveOrder/OrderState

Wire shapes verified against wallet-rpc-daemon v1.4.0 on testnet; orders.go at 100% statement coverage.

Adds the six order RPC wrappers the market-maker needs:
- CreateOrder / ConcludeOrder / FillOrder / FreezeOrder
- ListOwnOrders / ListAllActiveOrders (currency filters)
- OutputValue (Coin|Token tagged wire type), OwnOrder/ActiveOrder/OrderState

Wire shapes verified against wallet-rpc-daemon v1.4.0 on testnet;
orders.go at 100% statement coverage.
@github-actions

github-actions Bot commented Sep 14, 2026

Copy link
Copy Markdown

🔍 OpenCodeReview found 3 issue(s) in this PR.

  • ✅ Successfully posted inline: 1 comment(s)
  • 📋 Routed to summary by policy: 2 comment(s)

bug · low

📄 wallet/orders.go (L66-L69)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category bug)

UnmarshalJSON is less strict than MarshalJSON: a "Coin" value carrying a content "id" is silently accepted and TokenID reset to "", hiding a malformed daemon response. Consider rejecting non-empty content on Coin values for symmetry with the marshal-side validation.

💡 Suggested Change

Before:

	case "Coin":
		v.Coin, v.TokenID = true, ""
	case "Token":
		v.Coin, v.TokenID = false, raw.Content.ID

After:

	case "Coin":
		if raw.Content.ID != "" {
			return fmt.Errorf("wallet: coin output value must not carry a token id")
		}
		v.Coin, v.TokenID = true, ""
	case "Token":
		v.Coin, v.TokenID = false, raw.Content.ID

maintainability · low

📄 wallet/orders.go (L30-L37)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category maintainability)

The same anonymous wire-shape struct is declared twice in each branch, making the two call sites easy to diverge (e.g. if a field tag changes on one side only). Hoisting the wire struct to a small named local type (or a package-level type) and reusing it would keep the Coin and Token encodings in lockstep.

💡 Suggested Change

Before:

		return json.Marshal(struct {
			Type    string `json:"type"`
			Content struct {
				Amount Amount `json:"amount"`
			} `json:"content"`
		}{Type: "Coin", Content: struct {
			Amount Amount `json:"amount"`
		}{Amount: v.Amount}})

After:

	type wireCoin struct {
		Type    string `json:"type"`
		Content struct {
			Amount Amount `json:"amount"`
		} `json:"content"`
	}
	return json.Marshal(wireCoin{Type: "Coin", Content: struct {
		Amount Amount `json:"amount"`
	}{Amount: v.Amount}})

Comment thread wallet/orders.go Outdated
Comment on lines +70 to +73
if raw.Content.Amount != nil {
v.Amount = *raw.Content.Amount
}
return nil

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

bug · medium
UnmarshalJSON silently accepts a Coin/Token value with a missing or null "amount" (or a Token with an empty "id"), leaving OutputValue with a zero Amount. MarshalJSON validates TokenID but not Amount, so the contract is asymmetric: a malformed response, or a hand-built OutputValue{Coin:true} passed in CreateOrderParams.Ask/Give, is serialized as {"type":"Coin","content":{"amount":{}}} (Amount has omitempty on both fields) and the order is submitted with no amount at all. Since Amount "at least one of Atoms or Decimal must be set", validate here (and/or in MarshalJSON) and return an error instead of producing a zero-valued value.

Suggestion:

Suggested change
if raw.Content.Amount != nil {
v.Amount = *raw.Content.Amount
}
return nil
if raw.Content.Amount == nil || (raw.Content.Amount.Atoms == "" && raw.Content.Amount.Decimal == "") {
return fmt.Errorf("wallet: output value %q requires an amount", raw.Type)
}
v.Amount = *raw.Content.Amount
if !v.Coin && v.TokenID == "" {
return fmt.Errorf("wallet: token OutputValue requires TokenID")
}
return nil

… validation

- MarshalJSON/UnmarshalJSON both reject OutputValue without any amount
  (previously serialized {"amount":{}} or silently decoded a zero value —
  an order could have been submitted with no amount at all)
- UnmarshalJSON also rejects a Token value with an empty id (was
  marshal-only: asymmetric contract)
- TokenFilter("") now returns an error instead of building a filter that
  only fails with an opaque daemon-side RPC error

Review: OpenCodeReview run 34820118630 (1 inline bug·medium + 1 summary
bug·low); both addressed. orders.go 100% coverage, package 84.2%.
Comment thread wallet/orders.go
Comment on lines +101 to +106
// Coin filters carry no content on the wire (daemon-verified);
// token filters encode the bech32 id as content.
if f.Type == "Coin" {
return json.Marshal(struct {
Type string `json:"type"`
}{Type: f.Type})

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

bug · medium
MarshalJSON silently drops Content for any Coin-typed filter, including a zero-value-free misuse like &CurrencyFilter{Type: "Coin", Content: tokenID} or a mutated TokenFilter result, so ListAllActiveOrders would silently match the native coin instead of the intended token. Also, unlike the TokenFilter constructor, no validation ensures a non-Coin Type carries non-empty Content (e.g. &CurrencyFilter{Type: "Token"} marshals as a token filter with empty content). Prefer failing on these inconsistent states instead of silently encoding a possibly different currency.

Suggestion:

Suggested change
// Coin filters carry no content on the wire (daemon-verified);
// token filters encode the bech32 id as content.
if f.Type == "Coin" {
return json.Marshal(struct {
Type string `json:"type"`
}{Type: f.Type})
switch f.Type {
case "Coin":
if f.Content != "" {
return nil, fmt.Errorf("wallet: Coin filter must not carry content")
}
return json.Marshal(struct {
Type string `json:"type"`
}{Type: f.Type})
case "Token":
if f.Content == "" {
return nil, fmt.Errorf("wallet: Token filter requires a token id")
}
return json.Marshal(struct {
Type string `json:"type"`
Content string `json:"content"`
}{Type: f.Type, Content: f.Content})
}
return nil, fmt.Errorf("wallet: unknown currency filter type %q", f.Type)

@erubboli
erubboli merged commit 3dca090 into master Sep 14, 2026
1 check 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