fix(server): use address balances for Walrus uploads - #527
Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens Walrus upload transaction funding to require SUI/WAL address balances (and fail closed) across register/certify/metadata-transfer flows, preventing the Sui SDK from silently falling back to owned coin objects—including through retries and Enoki direct-sign fallback paths.
Changes:
- Add
enforceAddressBalanceCoinIntents()serialization plugin to preflight required address balances and reject any owned-coin resolution during build. - Introduce an
ADDRESS_BALANCE_WALLET_FALLBACK_POLICYto preserve legacy direct-sign fallback behavior while forcinggasMode: "addressBalance". - Apply address-balance enforcement across sidecar Walrus upload + metadata routes and Enoki sponsor execution; extend test coverage for the “no owned coin fallback” guarantee.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| services/server/scripts/walrus-upload.ts | Enforces address-balance intent checks and forces gas payment from address balance for CLI register/certify. |
| services/server/scripts/sidecar/wallet.ts | Adds ADDRESS_BALANCE_WALLET_FALLBACK_POLICY for legacy flows while retaining configured direct-sign fallback behavior. |
| services/server/scripts/sidecar/routes/walrus-upload.ts | Routes legacy upload transactions through ADDRESS_BALANCE_WALLET_FALLBACK_POLICY. |
| services/server/scripts/sidecar/routes/walrus-upload-journal.ts | Enforces address-balance intent checks on prepared register transactions. |
| services/server/scripts/sidecar/routes/walrus-metadata.ts | Forces metadata batch route to use address-balance fallback policy. |
| services/server/scripts/sidecar/enoki.ts | Enforces address-balance intents + clears gas payment when gasMode: "addressBalance". |
| services/server/scripts/sidecar/address-balance.ts | New enforcement helper to preflight address balances and reject owned-coin resolution. |
| services/server/scripts/tests/sidecar-query-helpers.test.ts | Adds regression tests for legacy fallback policy and “no owned coin fallback” enforcement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
services/server/scripts/sidecar/address-balance.ts:5
- Importing SUI_TYPE from ./config.js pulls in the sidecar config module at load time, which emits warnings (e.g. missing SEAL_SERVER_CONFIGS / SERVER_SUI_PRIVATE_KEYS) even when this helper is used by standalone scripts like walrus-upload.ts. This can add noisy stderr output and make it harder to distinguish real failures. Consider inlining the canonical SUI type here (or importing it from a side-effect-free module) so this helper stays dependency-light.
import type { Transaction } from "@mysten/sui/transactions";
import { normalizeStructTag } from "@mysten/sui/utils";
import { SUI_TYPE } from "./config.js";
harrymove-ctrl
left a comment
harrymove-ctrl
left a comment
There was a problem hiding this comment.
Request changes — gas-payment enforcement doesn't actually work
Verified the testnet evidence in this PR's description independently via sui client — it's genuine (certify tx A5mcrj2EtNiKtbvAvrE544sYUD2nGmtXXpk6KYzVk4u3 succeeded using address-balance/accumulator gas: Gas Payment: empty, Gas Object: 0x000...000). pnpm test also reproduces 175/175 clean.
But that only proves the happy path (sufficient address balance -> works). It doesn't prove the PR's core claim — "fail closed before the SDK can fall back to owned coin objects" — for gas payment specifically.
Root cause
enforceAddressBalanceCoinIntents (address-balance.ts) registers via transaction.addSerializationPlugin(...). Its post-next() check (line 69, gasData.payment.length > 0) is meant to catch owned-coin gas fallback, but it can't: per the SDK source (node_modules/@mysten/sui/src/transactions/Transaction.ts:796-798), build() calls prepareForSerialization() and #prepareBuild() as two separate, sequential awaits, not one chain. Serialization plugins (where this check lives) only run in the first phase. Gas-coin resolution (resolveTransactionPlugin) only runs in the second phase (#prepareBuild, Transaction.ts:819-824), strictly after our check has already returned. So by the time line 69 checks gasData.payment, gas hasn't been resolved yet — the check is structurally unable to observe an owned-coin gas fallback.
Since certify and metadata-transfer transactions carry no CoinWithBalance intents at all, this gas check is their only protection — meaning both are currently unprotected. This is reachable today: DURABLE_WALLET_FALLBACK_POLICY.directSignIfUnconfigured = true (wallet.ts:33) means any environment without ENOKI_API_KEY unconditionally hits the unprotected executeDirectSignedTransaction path (enoki.ts:218-233), and the standalone walrus-upload.ts CLI script has the same gap.
Suggested fix
Split the check: keep the owned-object/coin-intent rejection (lines 59-68, works correctly today) as a serialization plugin, but move the gas-payment check to transaction.addBuildPlugin(...) instead. Build plugins run before resolveTransactionPlugin in the plugin array, but since next() unwinds through the rest of the chain (resolveTransactionPlugin is appended last), a check placed after await next() in a build plugin will correctly observe the resolved gasData.payment — same pattern this file already uses, just registered in the phase that actually contains gas resolution.
Test gap that let this ship green
See inline comment below — the existing "fail instead of falling back to owned coins" test only calls prepareForSerialization(), never build(), so it never reaches the phase where the real bug lives. 175/175 passing didn't (and structurally couldn't) catch this.
Other findings (lower severity, not blocking but worth addressing alongside the fix above)
enoki.ts:fallbackPolicy.gasModeis optional with no type-level default forcing address-balance mode. A future caller that omitsfallbackPolicycompiles clean and silently reintroduces owned-coin gas fallback — no test would catch it.address-balance.ts:12: owned-object detection is a presence-diff (before/afternext()), not coin-type-aware. Not exploitable today, but it's the only guard certify/metadata-transfer would have even after the gas-phase fix, so worth hardening alongside it.
Scope note against the linked task ("review all upload-flow logic that depends on object-based balance"): register is solidly hardened; certify and metadata-transfer are the ones that still need it.
Happy to re-review once the gas-phase check is moved.
| `Address-balance upload resolved owned coin objects: ${newlyResolvedOwnedObjects.join(", ")}`, | ||
| ); | ||
| } | ||
| if (transactionData.gasData.payment && transactionData.gasData.payment.length > 0) { |
There was a problem hiding this comment.
This check can't fire in practice — gasData.payment isn't resolved yet at this point. build() runs prepareForSerialization() (this plugin's phase) fully to completion before #prepareBuild() even starts, and gas-coin resolution (resolveTransactionPlugin) only happens in that second phase. See the top-level review comment for the SDK source citation.
Suggest moving this specific check into transaction.addBuildPlugin(...) so it runs after gas resolution instead.
| assert.equal(withdrawals.length, 2, "WAL payment and relay SUI tip both use address balances"); | ||
| }); | ||
|
|
||
| test("address-balance uploads fail instead of falling back to owned coins", async () => { |
There was a problem hiding this comment.
This test's title says "fail instead of falling back to owned coins", but it only calls transaction.prepareForSerialization(...) (lines 268-274), never .build(...). Gas-coin resolution only happens inside #prepareBuild, which prepareForSerialization never reaches — so this test can only exercise the coin-intent-resolved-to-owned-object case (which does correctly reject today), not the gas-payment fallback case the PR summary claims is covered.
Suggest adding a case that mocks gas resolution to return an owned coin and asserts transaction.build({ client }) rejects the same way. With the current code, that new test would fail — that's the actual bug this PR needs to fix.
hungtranphamminh
left a comment
hungtranphamminh
left a comment
There was a problem hiding this comment.
Verified 4bf0de1e addresses the review. The gas-payment guard is moved into a build plugin (addBuildPlugin, checked after await next()), so it runs in the #prepareBuild phase where resolveTransactionPlugin resolves gas — it can now actually observe an owned-coin gas payment. The new "...reject owned gas resolved during build" test mocks gas resolution to an owned coin and asserts build() rejects, closing the test gap. Both secondary items are handled too: gasMode is now required + defaults to addressBalance, and the newly-resolved-object check is coin-type-aware (only rejects 0x2::coin::Coin).
Also confirmed the enforcement reaches certify and metadata-transfer at runtime — enforceAddressBalanceCoinIntents is applied at the top of executeWithEnokiSponsor before any branch, so it covers the direct-sign fallback paths as well as the sponsor path.
LGTM.
harrymove-ctrl
left a comment
harrymove-ctrl
left a comment
There was a problem hiding this comment.
LGTM, approving.
|
Closing — no longer needed. Address-balance upload work is not going forward on this branch. |
Summary
Verification
pnpm testinservices/server/scripts: 175/175 passedsidecar-server.tsandwalrus-upload.ts: passedgit diff --check: passed8849821a, re-review completed with no findingsLive testnet evidence
A one-epoch upload completed end to end using wallet address balances, then the payload was read back through the Walrus client:
rwPd-5ESC_b3SS8ChY1TF-J6iygqMy5BFNh_RBU9r6g0x8b697e6c360eac8204e788d8d006be976b8833090ceddbce8e06af0c4e58e482A5mcrj2EtNiKtbvAvrE544sYUD2nGmtXXpk6KYzVk4u3MemWal final address-balance testnet upload 1785814830Testnet WAL was moved from owned coin storage into the writer address balance before the upload:
DWU8L6GAxcqehaiovDyeJABtNb53wEB43m4vdLvcjm6gDeployment note
This is not an API schema change, but it is an operational behavior change. Writer wallets must have sufficient SUI and WAL address balances; owned coin fallback is intentionally rejected. The current mainnet wallet pool must be funded through the address-balance distribution flow before deployment.