Skip to content

fix(taip-5): require for on address agents - #15

Merged
momilo merged 1 commit into
mainfrom
fix/taip5-require-agent-for
Sep 11, 2026
Merged

momilo merged 1 commit into
mainfrom
fix/taip5-require-agent-for

Conversation

@thomasjsk

@thomasjsk thomasjsk commented Sep 11, 2026 •

Copy link
Copy Markdown
Collaborator

TAIP-5 says every agent MUST carry for — the DID of whoever it acts on behalf of. This library tagged the field omitempty and never checked it, so agents went out without one and nothing complained. That matters most on a settlement address: for is what tells a receiver whether a customer holds the keys or their VASP custodies it, and the two Notabene nodes read a missing one in opposite directions — one assumes custodial, the other assumes self-hosted and demands an ownership proof.

Every constructor that carries agents now validates them — including NewLockMessage, NewUpdateAgentMessage and NewReplaceAgentMessage, which validated nothing at all before. (#13 already fixed the marshalling half, so an ownerless for omits the key rather than shipping null.)

Required on addresses, not on everything

for is enforced on the blockchain-address roles (SourceAddress, SettlementAddress) rather than on every agent, and this is a deliberate divergence from the letter of TAIP-5.

The spec has no way to say that who owns an agent is not established yet, which is a state real flows pass through: an address can be seen before anybody has resolved who custodies it, and an institution can join a transaction before it says whose behalf it acts on. Enforcing for there would force implementations to invent an owner — and an invented for is worse than an absent one, because a receiver stores it as fact and it decides whether a wallet must prove ownership or must not.

Whoever supplies a blockchain address does know whose address it is, so that is where the line sits. An empty DID inside for is rejected whatever the role.

Worth settling upstream: should for be strictly REQUIRED, or should an agent pending ownership resolution be expressible? The spec is already inconsistent here — the JSON schemas list only @id as required, while the prose marks for REQUIRED too.

Send-side only

ParseBody is unchanged and still accepts inbound agents without for, whatever the role. Other implementations treat the field as optional — the TypeScript reference types declare it for?: string, and at least one inbound validator checks it only "when present" — so rejecting on receive would drop traffic that is valid today.

UpdateParty used the wrong field name

TAIP-6 calls the field partyType; this library called it role. A conformant peer looking for partyType found nothing and could not tell which party the update was about, so these messages were effectively dropped. Renamed, with inbound bodies still reading role so peers on the old spelling keep working.

Deliberately not here

  • Lock and RFQ keep their names. The spec on main defines #Lock (TAIP-17) and #RFQ (TAIP-18); the rename to Escrow/Exchange exists only on an unmerged branch, so following it now would break interop rather than fix it.
  • lei:leiCode keeps its prefix. TAIP-11 contradicts itself — the JSON schemas say lei:leiCode, one prose example says leiCode — and this library follows the schemas.
  • amount on Transfer stays unvalidated. TAIP-3 makes it required for fungible tokens but optional for NFTs, and an NFT is not reliably detectable from a CAIP-19 string, so a blanket check would reject valid NFT transfers.

Breaking

An address agent without for now returns an error instead of going out malformed, and UpdatePartyBody.Role is now PartyType.

@thomasjsk
thomasjsk force-pushed the fix/taip5-require-agent-for branch from 3209ac6 to f1a4d06 Compare September 11, 2026 18:04
@thomasjsk thomasjsk changed the title fix(taip-5): require the for attribute on every agent fix(taip-5): require for on address agents, and stop dropping it Sep 11, 2026
@thomasjsk
thomasjsk marked this pull request as ready for review September 11, 2026 18:10
@thomasjsk
thomasjsk force-pushed the fix/taip5-require-agent-for branch from f1a4d06 to 2f30983 Compare September 11, 2026 18:14
@thomasjsk thomasjsk changed the title fix(taip-5): require for on address agents, and stop dropping it fix(taip-5): require for on address agents Sep 11, 2026
@thomasjsk
thomasjsk force-pushed the fix/taip5-require-agent-for branch from 2f30983 to e5e1518 Compare September 11, 2026 18:17
TAIP-5 marks `for` REQUIRED on every agent, and this library never
checked it. Agents went out without one and nothing complained —
including settlement addresses, where `for` is the only thing telling a
receiver whether a customer holds the keys or their VASP does.

Every constructor that carries agents now validates them: Transfer,
Payment, Connect, Quote, RFQ, AddAgents, Lock, UpdateAgent and
ReplaceAgent. The last three validated nothing at all before.

`for` is enforced on the blockchain-address roles rather than on every
agent. The spec has no way to say that who owns an agent is not
established yet, which is a state real flows pass through — an address
can be seen before anybody has resolved who custodies it, and an
institution can join a transaction before it says whose behalf it acts
on. Demanding `for` there would mean inventing an owner, and an invented
one is worse than an absent one: a receiver stores it as fact, and it
decides whether a wallet must prove ownership or must not. Whoever
supplies an address does know whose it is, so that is where the line sits.
An empty DID inside `for` is rejected whatever the role.

Validation is send-side only. ParseBody accepts inbound agents without
`for` regardless of role, so peers that do not set it keep working.

Also retags UpdatePartyBody.Role to `partyType` per TAIP-6. The `role`
spelling matched no other implementation, so these messages were silently
ignored by conformant peers; inbound bodies still read `role` as a
fallback.
@thomasjsk
thomasjsk force-pushed the fix/taip5-require-agent-for branch from e5e1518 to 62d48a1 Compare September 11, 2026 18:19
@momilo
momilo merged commit 5ce9b27 into main Sep 11, 2026
3 checks 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.

2 participants