fix(token): address review findings from #319 (DAR vetting, accept-leg auth, JSON parity) - #321
Merged
Merged
Conversation
zheli
force-pushed
the
fix/token-dar-review-followups
branch
from
August 19, 2026 15:58
d9c9bf8 to
de2d4eb
Compare
`dar upload --all-participants` re-spelled the literal
{"sv","app-provider","app-user"} while the token DAR path derived the
same list from splice.AllRoles(). A comment asserted the two must match,
but nothing enforced it, so the two fan-out paths could silently drift in
which participants they target or in what order.
Both now derive from splice.AllRoleNames(), making the claim structural.
…arops The token DAR fan-out carried its own dialling and vetting logic parallel to internal/localnet/darops, and its probe was wrong in ways that could report success while the participant could not host the package: - It called the ledger API's ListKnownPackages, which reports upload, not vetting, so "Vetted ..." overstated what had been checked. It now uses the admin API's ListDars, the same probe darops already relies on. - It matched packages by name alone, so the pinned splice-util-token-standard-wallet 1.1.0 was satisfied by an installed 1.0.0 that lacks BatchingUtilityV2. Matching is now name and version. - It issued one list call per package per role (21 per create). It now issues one per role before uploads and one after. - A failing list RPC was indistinguishable from "package absent", causing redundant uploads and a misleading "still not vetted after upload". List errors now propagate with the failing role and host. - An interrupted cache write could leave a truncated DAR that later runs would trust. Cache entries are now written to a temp file in the same directory and renamed into place. Extracting EnsureVetted into darops lets the token path and `dar upload` share one implementation instead of drifting.
Both auto-accept paths rebuilt a LedgerConn from the options to target the receiver's participant, but the rebuilt value omitted Token. The accept leg therefore dialled unauthenticated whenever the caller had supplied a JWT, failing against an auth-on LocalNet. Both blocks now pass the in-scope conn, which already carries Token.
Which participants a create vetted was only ever printed as prose, so neither `token create --format json` nor POST /api/tokens could report it. Scripts and the Web UI had no way to tell a fully vetted create from a registry-only one. Both surfaces now encode a shared types.TokenCreateResponse carrying schema_version and vetted_roles, so they cannot drift. TokenRef stays embedded rather than nested, keeping the instrument's fields where they have always been on the wire. vetted_roles is omitted for a registry-only create, which vets nothing.
AGENTS.md requires the proposal-deviation entry to track user-facing behaviour. The existing `token create` entry described a ledger-port failure mode that is now an admin-port one, and predates both the vetted_roles JSON field and the atomic DAR cache write.
resolvePackageID and discoverTokenSurfaces both call the ledger API's ListKnownPackages, which proves a package was uploaded to the participant. It does not prove the package is vetted -- vetting is a topology fact, and darops.EnsureVetted reads it through the admin API. Both functions and the four user-facing errors that inherit from them nonetheless told the operator a package was or was not "vetted". That sends someone debugging a routing failure to look at vetting state that was never actually checked, which is the opposite of what the message should do. Reword the claims to match what the call proves, and record on discoverTokenSurfaces which API to reach for when the vetting state itself is what matters. No behaviour change: the same packages are found by the same call.
zheli
force-pushed
the
fix/token-dar-review-followups
branch
from
August 19, 2026 16:09
de2d4eb to
4ba3b55
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #319, which merged with seven non-blocking review findings deferred. All seven are addressed here.
Findings addressed
1. Duplicated the
daropsfan-out. The token DAR path carried its own dialling and vetting logic parallel tointernal/localnet/darops/. Extracteddarops.EnsureVettedand deleteddarClient,dialDARClient,packageKnown,vetTokenDARsOn.2. "Vetted" overstated what was checked. The probe used the ledger API's
ListKnownPackages, which reports upload, not vetting. It now uses the admin API'sListDars— the same probedaropsalready relied on — so the word is accurate.This also upgraded matching from name-only to name and version. Verified against a live instance: the old match would accept an installed
splice-util-token-standard-wallet1.0.0 for the pinned 1.1.0, which is the version carryingBatchingUtilityV2.3. 21 list RPCs per create. Was one list call per package per role. Now one per role before uploads and one after.
4. Non-atomic DAR cache write. An interrupted run could leave a truncated
.darthat later runs would trust. Cache entries are written to a temp file in the same directory and renamed into place.5. List errors read as "package absent". A failing list RPC was indistinguishable from a missing package, causing redundant uploads and a misleading "still not vetted after upload". Errors now propagate with the failing role and host.
6. Accept leg dropped the auth token. Both auto-accept paths rebuilt a
LedgerConnwithoutToken, so the accept leg dialled unauthenticated whenever a JWT was supplied. Both now pass the in-scopeconn.7.
vettedRoleswas prose-only. Neithertoken create --format jsonnorPOST /api/tokenscould report which participants were vetted. Both now encode a sharedtypes.TokenCreateResponsewithschema_versionandvetted_roles.TokenRefstays embedded, so the instrument's fields keep their wire position;vetted_rolesis omitted for a registry-only create.Also included
dar upload --all-participantsre-spelled the role literal while the token path derived it fromsplice.AllRoles(), with only a comment asserting the two must match. Both now derive fromsplice.AllRoleNames().Tests
New regression coverage for the three defects that had none: atomic cache write, one-list-per-role, and list-error propagation, plus name+version matching and the JSON response shape.
make lintreports0 issues.Pre-existing failure, not from this PR
TestStart_StoppedNoContainers202ininternal/ui/handlersfails withstatus = 204, want 202. Confirmed by running it in a pristine worktree atorigin/mainwith zero changes, where it fails identically. It is currently broken onmainand likely wants its own issue.