Vultisig/421 payout

[evgensheff]

No description provided.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch vultisig/421-payout

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[gomesalexandre]

CI red — tests job fails with HTTP 500. This is an old run (Feb 17). Build is passing.

should-fix: The tests failure needs to be investigated — HTTP 500 from a test server is a hard blocker. Either rebase to pick up any fixes since Feb 17, or identify if this is an environment flake and re-trigger.

Not approving with red CI.

[gomesalexandre]

smoke test has been failing since Feb 2026, PR is stale. @evgensheff can you rebase + fix the test failure and re-request review?

[gomesalexandre]

solid plugin proposal pipeline. the postgres constraint classification, email async pattern, and IsConfigured guard are all well-done. a few things to lock down:

pluginID path param - not sanitized before DB / URL

should-fix: ValidateProposedPlugin takes pluginID directly from the URL param and passes it to db.IsProposedPluginApproved as a raw string, then the admin URL embeds it via url.PathEscape(pluginID) in sendProposalNotification. the DB query uses a parameterized $1 so SQL injection is not a concern, but there's no validation that pluginID matches the expected format (e.g. ^[a-z0-9-]+$). a crafted ID like ../../../etc/passwd won't break the SQL but could cause issues in URL generation or log injection. quick win: add a max-length check and reject non-alphanumeric-hyphen chars before the DB call.

IsProposedPluginApproved status check vs 'listed' status

q: IsProposedPluginApproved checks status = 'approved'. once a plugin is published it transitions to 'listed'. should a plugin developer be able to use the approved-validation endpoint after their plugin has already been listed (and is paying)? if the payment flow calls this endpoint as a pre-flight check and the plugin moved to 'listed' in between, it'll return 404. probably should be status IN ('approved', 'listed') if the intent is "has this plugin been cleared to accept payment".

async email goroutines - fire-and-forget with no observability

suggestion: the goroutines in SendXxxAsync have a 30s timeout but no metrics, no retry, and no dead-letter queue. the TODO comment acknowledges this. fine for now but worth a follow-up issue - a transient Mandrill outage silently drops the notification with only a log line. the TODO is already in the code, just flag it for a linked issue.

MandrillAPIKey in JSON response

suggestion: PortalEmailConfig has json:"mandrill_api_key,omitempty" which means it'll appear in any endpoint that serializes the config struct. verify this struct is never returned in an API response - if it is, that's a key leak. looks like it's only used internally but worth a double-check.

praise: classifyProposalConstraintViolation is a clean pattern for surfacing meaningful errors from postgres constraint names without leaking internal DB details to the client. good call using pgerrcode constants instead of raw strings.

[gomesalexandre]

verifier#565 — plugin payout address

Straightforward schema + API extension. Walked every code path in UpdatePlugin.

---

should-fix: payout_address validation only accepts EVM hex addresses - THORChain/Bitcoin plugin devs are blocked

if !common.IsHexAddress(req.PayoutAddress) {
    return c.JSON(http.StatusBadRequest, map[string]string{"error": "invalid payout address format"})
}

common.IsHexAddress is go-ethereum's check - accepts 0x... 20-byte hex only. A plugin developer on THORChain or Bitcoin (plausible given this repo handles multi-chain plugins) cannot set a native-chain payout address. If the intent is EVM-only for now, document it in the request type (validate:"required,eth_addr" or a comment). If it should be multi-chain eventually, the validation needs a chain-type field or a more permissive check now. Either way the current silent rejection with a generic "invalid payout address format" is confusing.

---

should-fix: normalizedExistingPayout uses HexToAddress on potentially non-EVM string

normalizedExistingPayout = common.HexToAddress(existingPlugin.PayoutAddress.String).Hex()

If a future migration or direct DB write stores a non-EVM address (bech32, base58), HexToAddress silently returns a zero address. The oldValue check in the signed message would then compare against 0x0000...0000 and reject the request with a confusing "payout address old value does not match" error. Should check IsHexAddress on the existing value before calling HexToAddress on it, or use a consistent type constraint at the DB layer.

---

q: maskPayoutAddress is only called in the log handler, not returned in the response - confirm intentional

logFields["new_payout_address"] = maskPayoutAddress(plugin.PayoutAddress.String)

The plugin returned in the JSON response at line ~1278 includes PayoutAddress unmasked. Is that intentional? Admin-only write, so the admin should be able to see it, but confirm there's no path where non-admins get the full address back through the listPlugins / getPluginByID routes.

Looking at queries.Plugin - PayoutAddress pgtype.Text is included in GetPluginByID, ListPlugins, ListPluginsByOwner. The types.Plugin has it as string, omitempty. So it's public in list/get responses. If that's intentional (payout addr is public info), fine - but worth a deliberate comment since this is financial-adjacent.

---

q: what's the idx_plugins_payout_address index actually for?

CREATE INDEX idx_plugins_payout_address ON plugins(payout_address)
WHERE payout_address IS NOT NULL;

There's no query in this PR that filters by payout_address. If the index is forward-looking (for a future payout sweep query), fine - but call that out in the migration comment. If it's not needed now, skip it and add it when the query exists.

---

suggestion: migration file missing newline at EOF

CREATE INDEX idx_plugins_payout_address ON plugins(payout_address)
WHERE payout_address IS NOT NULL;
-- +goose StatementEnd

No trailing newline. Minor but diff tooling will flag it.

---

praise: the EIP-712 signed-message guard for payout address changes is solid

Requiring the old value in the signed message (like serverEndpoint changes) prevents replay attacks where an attacker replays an old signed update after the payout address has been changed. The normalization step (via HexToAddress) before the comparison ensures 0xaBC and 0xabc match correctly.

---

Verdict: COMMENT - no blockers (no on-chain arithmetic, no payout actually sent in this PR, no double-payout risk - it's just storing the field). The two should-fix items are correctness issues for future-proofing but don't endanger funds today since payout triggering isn't in scope here.

[gomesalexandre]

multi-lane pass: Claude correctness + adversarial threat-model. requesting changes.

---

Summary

Big PR - adds the full plugin-proposal lifecycle (submit -> approve -> publish) with payout-address management, email notifications via Mandrill, and a listing-fee check. The bones are solid: the state-machine is clear, the S3 cleanup on partial failure is present, and the payout address uses EIP-55 normalization. A few things need fixing before this ships.

---

preferably-blocking findings

1. ApprovePluginProposal lets the caller supply the PublicKey - they can approve anyone's proposal

server.go ApprovePluginProposal:

proposal, err := s.queries.UpdateProposedPluginStatus(ctx, &queries.UpdateProposedPluginStatusParams{
    PublicKey:     req.PublicKey,   // <-- comes from request body
    ...

The UpdateProposedPluginStatus SQL filters on public_key, so passing an arbitrary key from the request body means the admin can approve any developer's proposal without that dev being involved - or, more dangerously, a future refactor could inadvertently match something unexpected. The public_key constraint here is meant to scope the row, but the approver already found the proposal by id. Why is PublicKey in the approve request body at all? The route is POST /admin/plugin-proposals/:id/approve. The only thing needed is the :id param. Recommend: drop req.PublicKey from ApprovePluginProposalRequest, look up the proposal by ID first, and use the stored public_key from the DB row in the SQL call. As-is, an approver who sends a wrong publicKey will silently get a 409 (looks like it failed to the caller but the approval was just skipped), which is spooky.

2. Listing-fee check is skippable - listingFeeClient nil means free publish

server.go PublishPluginProposal:

if s.listingFeeClient != nil {
    paid, err := s.listingFeeClient.IsListingFeePaid(ctx, pluginID)
    ...
}

If DeveloperServiceURL is not configured (empty string), listingFeeClient is nil and the fee check is silently skipped - any approved plugin gets published for free. In prod this depends entirely on the env var being set. If someone misses it in a deploy, every plugin goes live gratis. This should either hard-fail at startup when listingFeeClient is nil and the publish route is called, or the behavior should be explicitly documented + a warning logged at startup. As-is it's a silent fund-safety bypass.

3. PublishPlugin SQL does not include payout_address - it's dropped on publish

sqlc/plugins.sql / generated plugins.sql.go:

INSERT INTO plugins (id, title, description, server_endpoint, category)
SELECT plugin_id, title, description, server_endpoint, category
FROM proposed_plugins
WHERE plugin_id = $1 AND status = 'approved'

proposed_plugins doesn't have a payout_address column, so this is fine for the proposal->listing transition. But the PR title is "payout" - the whole point is payout address management. The plugin's payout_address starts as NULL after publish. That's probably fine intentionally (dev sets it post-publish via UpdatePlugin), but nowhere in the proposal flow does the developer provide a payout address, and there's no documentation or TODO. If the intent is "developer sets it after listing via the signed UpdatePlugin flow", that should be a comment in the code. If the intent is "they set it during proposal", the field is missing from CreateProposedPluginRequest. Clarify the intended data flow.

4. Race condition - isPluginIDAvailable check before DB insert is not atomic

proposed_plugin_handlers.go CreatePluginProposal:

available, err := s.isPluginIDAvailable(ctx, req.PluginID)
// ...
// no lock held here
proposal, err := s.db.CreateProposedPlugin(ctx, tx, ...)

The availability check runs outside the transaction. Two concurrent requests with the same plugin_id can both pass the check and then race to the INSERT. The duplicate-key constraint on proposed_plugins_pkey catches it and you do handle violationPluginIDTaken - so data integrity holds. But this also means you're doing two extra SELECT queries (PluginIDExistsInPlugins + PluginIDExistsInProposals) that are inherently racy and could mislead the user. The 409 from constraint violation is the correct path. The pre-check is fine as a UX optimisation but should be treated as advisory, not authoritative.

Not a blocker since the constraint handles it, but the comment should be explicit that the pre-check is best-effort.

---

should-fix findings

5. UpdateProposedPluginStatus SQL still filters on public_key for an admin-only operation

UPDATE proposed_plugins
SET status = $1, updated_at = NOW()
WHERE public_key = $2 AND plugin_id = $3 AND status = $4

This is the query used in ApprovePluginProposal. Filtering by public_key for an approval that doesn't need it creates the footgun in finding #1. The query should only filter by plugin_id and status. If you want to RETURNING the proposal (including public_key) for sending the email, do a separate SELECT or just drop the public_key WHERE clause.

6. sendEmailTo is a test helper leaking into production code

In email_test.go:

func (s *EmailService) sendEmailTo(ctx context.Context, url string, ...) error {
    s.mandrillURL = url
    return s.sendEmail(ctx, ...)
}

This method is defined in _test.go but because it's in package portal (not package portal_test) it could technically be called from non-test code. Worse, it mutates s.mandrillURL which is a shared field - if called concurrently (the async goroutines!) this is a data race. Test helpers should not mutate production struct fields. Use an injected URL field instead, or make the test server point svc.mandrillURL = server.URL directly (which the async test already does correctly).

7. Migrations missing -- +goose Down body

20260212000001_create_proposed_plugins.sql:

-- +goose Down

Empty. If a migration needs to be rolled back in prod (bad deploy, hotfix), there's nothing to undo the schema changes - ENUMs, tables, indexes all get left behind. Same for 20260217000000_add_plugin_payout_address.sql (no Down section at all - missing the trailing newline too). Add Down migrations or explicitly comment -- intentionally irreversible so the next person doesn't wonder.

8. Body limit is 35MB but max image payload is much smaller

server.go:

e.Use(middleware.BodyLimit("35M"))

CreateProposedPluginRequest allows max 9 images × 2MB = 18MB + JSON overhead. 35MB is nearly 2x the max. This is fine for the portal server but the api server (used by vault clients) presumably also benefits from a body limit - is there one? Not introduced in this PR but worth noting.

9. GetMyPluginProposals / GetAllPluginProposals do N+1 image queries

For each proposal returned, there's a separate ListProposedPluginImages query. For an admin listing all proposals, that's O(n) additional queries. Fine at MVP scale but will hurt in prod. Should be a JOIN or a batched query. At minimum add a TODO.

10. proposed_plugin_images.image_type is TEXT with a CHECK constraint - should be an ENUM

The migration uses image_type TEXT NOT NULL with a CHECK constraint, while plugin_images (the production table) presumably uses the PluginImageType type. Inconsistency between tables that get merged on publish. On the copy path (PublishPluginProposal), the image_type string is passed verbatim - fine since the CHECK constraint validates it, but for type safety and consistency it should be the same ENUM type.

11. S3 Copy uses url.PathEscape on the bucket/key separator

plugin_assets.go:

copySource := fmt.Sprintf("%s/%s", s.cfg.Bucket, url.PathEscape(srcKey))

S3 CopySource expects the format bucket/key where the key is URL-encoded per AWS docs, but url.PathEscape will encode the / separators inside the key, which is correct. However if the bucket name has special characters (unlikely but possible), this could break. More importantly: if srcKey contains % characters already, double-encoding will occur. Worth a comment explaining why url.PathEscape was chosen over url.QueryEscape or raw.

---

suggestion findings

12. IsConfigured() doesn't check FromName

config.go:

func (c PortalEmailConfig) IsConfigured() bool {
    return c.MandrillAPIKey != "" && c.FromEmail != "" && len(c.NotificationEmails) > 0
}

FromName is not validated. Mandrill will send with an empty From Name, which looks sketchy to recipients. Minor but should at least log a warning at startup if FromName is empty.

13. ValidateProposedPlugin in api/proposed_plugin.go - the VaultAuthMiddleware is on a GET endpoint that vaults poll

server.go:

pluginsGroup.GET("/proposed/validate/:pluginId", s.ValidateProposedPlugin, s.VaultAuthMiddleware)

This endpoint presumably gets called by the vault app before launching a proposed plugin. Using VaultAuthMiddleware here means the vault must already have signed auth context. That seems intentional, but the check only returns {"valid": true/false} - returning 404 vs 200 with valid: false is an inconsistency (404 is returned when not approved, but valid: false would be cleaner for a validate endpoint). Callers have to handle both a 404 body and a 200 body.

14. maskPayoutAddress is EVM-only

func maskPayoutAddress(address string) string {
    if len(address) == 42 && strings.HasPrefix(address, "0x") {
        return address[:6] + "..." + address[len(address)-4:]
    }
    return address  // returns unmasked for non-EVM
}

For non-EVM payout addresses (if this ever supports them), the address is logged unmasked. Return a generic mask for the unknown-format case.

15. IsListingApprover, IsPortalAdmin, and IsStagingApprover are three identical queries

portal_approvers.sql:

All three execute SELECT EXISTS(SELECT 1 FROM portal_approvers WHERE public_key = $1 AND active = TRUE). They're aliases for the same check. Consolidate or at minimum add a comment explaining the distinction (staging vs listing vs admin are currently synonymous).

16. proposed_plugin_handlers.go defines maxMediaImagesProposal = 7 but the validator has max=9

const maxMediaImagesProposal = 7
// ...
Images []ImageData `json:"images" validate:"required,min=1,max=9,dive"`

The max=9 validator counts total images (logo + banner + thumbnail + media), while maxMediaImagesProposal = 7 counts only media. 1 logo + 1 banner + 1 thumbnail + 7 media = 10 images, which exceeds the max=9 struct-level validator. The two constraints are inconsistent. Either bump the struct validator to max=10 or document why the total cap is 9.

---

q: questions

• q: ApprovePluginProposal takes publicKey in the request body - is this intentional? What's the use case for an approver specifying the developer's key explicitly vs just approving by proposal ID?

• q: The portal_approvers table has no seeding mechanism shown in this PR. How does the first approver get bootstrapped? Via a CLI tool? The added_via = 'bootstrap' enum value suggests there's a bootstrap path somewhere.

• q: IsProposedPluginApproved returns false for status listed - meaning once a proposal is published, re-validation by vault would return "not approved". Is that intentional? The listed plugin is already live at that point so it probably should return true (or there's a separate approval check for listed plugins).

• q: Why is pgerrcode marked as // indirect in go.mod? It's directly imported in proposed_plugin_handlers.go. Should be a direct dependency.

---

praise

• praise: The EIP-55 normalization for payout addresses (common.HexToAddress(req.PayoutAddress).Hex()) + the signed old/new value check is solid. Good use of the existing EIP-712 update flow to protect a sensitive field.

• praise: The cleanup() / rollbackS3() functions before DB rollback are well-structured. S3 and DB are separate systems and the ordering (DB tx rollback + S3 cleanup) handles partial failures correctly.

• praise: sanitizeValidationError strips internal field names from validator errors before sending to the client. Good hygiene.