feat(vault): contract-side admin events + RPC admin-history reader - #698
Conversation
|
@ZacLou is attempting to deploy a commit to the Collins' projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@ZacLou this PR now has merge conflicts with main after today's admin-dashboard and vault contract merges. Could you rebase onto the latest main and resolve them? |
b7af5e8 to
7c01dc0
Compare
|
Rebased onto latest main — all conflicts resolved. The vault contract change was straightforward: upstream's For |
|
Hi @collinsezedike, the Vercel deployment check is failing with 'Authorization Required' for team 'Collins''' projects'. As an external contributor I don't have permission to authorize Vercel for your team. Could you please click the authorize link in the failing check? The code is ready for review. Thanks! |
collinsezedike
left a comment
There was a problem hiding this comment.
The vault-side event emissions build cleanly and are low-risk: every publish() call is added after the existing require_admin-gated state mutation, none of that logic is touched. The new RPC-based admin-history reader in admin-history.ts is where the real problems are.
Running pnpm --filter @meridian/stellar-sdk-helpers build on this branch fails to compile this file outright: Server and SorobanRpc are not exports of the installed @stellar/stellar-sdk (14.6.1), ScVal has no .obj() method in this version, and the timestamp field trips exactOptionalPropertyTypes. Beyond the compile errors, the hardcoded topic filter is also wrong: I decoded it, AAAABWFkbWluAAAA is 00 00 00 05 61 64 6d 69 6e 00 00 00, missing the SCV_SYMBOL discriminant (15) entirely. The correct encoding of ScVal::Symbol("admin") is AAAADwAAAAVhZG1pbgAAAA== (verified by encoding it directly with this repo's own installed SDK). Even patched to compile, this filter would never match a single real event from the vault contract, so getRpcAdminHistory would silently return an empty list forever.
| @@ -1,6 +1,13 @@ | |||
| import { Address, xdr } from "@stellar/stellar-sdk"; | |||
| import { Address, Server, xdr, SorobanRpc } from "@stellar/stellar-sdk"; | |||
There was a problem hiding this comment.
Neither Server nor SorobanRpc exists on @stellar/stellar-sdk 14.6.1, confirmed by tsc: "has no exported member 'Server'" and "has no exported member named 'SorobanRpc'". This SDK exposes the RPC client as rpc.Server and the event type as rpc.Api.EventResponse (import { rpc } from "@stellar/stellar-sdk").
There was a problem hiding this comment.
Fixed in b66e84d. Now import { rpc } from "@stellar/stellar-sdk", with new rpc.Server(rpcUrl) as the client and rpc.Api.EventResponse as the event type. Address and xdr are unchanged.
One thing worth flagging: because Server / SorobanRpc did not resolve, server and event were typed as any, which suppressed every downstream type error in this function. Once the import was correct, tsc surfaced several more mismatches against the real 14.6.1 types. All are fixed in the same commit:
event.ledgeris the field name, notledgerSequenceresponse.cursoris the pagination token, notnextCursortopicisxdr.ScVal[], notstring[]valueis anxdr.ScVal, not a{ xdr() }wrapper, so the base64 round-trip is goneApi.GetEventsRequestis a discriminated union that rejects a request carrying bothstartLedgerandcursor, so the request is now built in two branches depending on which pagination mode is in use
The Horizon reader and the vault contract are untouched.
| { | ||
| type: 'contract', | ||
| contractIds: [vaultContractId], | ||
| topics: [['AAAABWFkbWluAAAA']], // xdr.ScSymbol("admin") base64 |
There was a problem hiding this comment.
This base64 is wrong. Decoded, it's 00 00 00 05 61 64 6d 69 6e 00 00 00, missing the leading ScVal discriminant for SCV_SYMBOL (15), so it isn't valid ScVal XDR at all, and won't match the topic the vault actually publishes. The correct value, produced by xdr.ScVal.scvSymbol("admin").toXDR("base64") in this repo's own SDK, is AAAADwAAAAVhZG1pbgAAAA==. As written this filter matches nothing, ever.
There was a problem hiding this comment.
Fixed, and agreed this was the one that mattered. Confirmed locally with this repo's installed SDK (@stellar/stellar-sdk 14.6.1, stellar-base 14.1.0):
> xdr.ScVal.scvSymbol("admin").toXDR("base64")
'AAAADwAAAAVhZG1pbgAAAA=='
> xdr.ScVal.scvSymbol("admin").toXDR("hex")
'0000000f0000000561646d696e000000'
Matches your value exactly. For the record, the old literal does not decode to the wrong discriminant but to an entirely different arm:
> xdr.ScVal.fromXDR("AAAABWFkbWluAAAA", "base64").switch().name
'scvU64'
so it was structurally the wrong ScVal, not just a bad prefix — as you say, it would never have matched a real vault event.
Rather than paste the literal back in, the topic is now derived at module load:
const ADMIN_TOPIC_XDR: string = xdr.ScVal.scvSymbol("admin").toXDR("base64");That way it cannot drift from the encoding the same SDK decodes. I verified the outgoing request carries topics: [["AAAADwAAAAVhZG1pbgAAAA=="]] and that all five action topics (paused, transfer, accept, adapter, migrate) now parse.
| const base: Omit<RpcAdminAction, 'payload'> = { | ||
| action, | ||
| ledgerSequence: event.ledgerSequence, | ||
| timestamp: event.ledgerClosedAt ? new Date(event.ledgerClosedAt) : undefined, |
There was a problem hiding this comment.
tsc rejects this under exactOptionalPropertyTypes: timestamp?: Date doesn't accept an explicit undefined value, only omission of the key. Either widen the type to timestamp?: Date | undefined or build the object without the key when there's no ledgerClosedAt.
There was a problem hiding this comment.
Fixed using the second option — the key is only constructed when ledgerClosedAt is present, so undefined is never explicitly written:
const base: Omit<RpcAdminAction, "payload"> = {
action,
ledgerSequence: event.ledger,
...(event.ledgerClosedAt
? { timestamp: new Date(event.ledgerClosedAt) }
: {}),
};I kept timestamp?: Date narrow rather than widening to Date | undefined: callers still read Date | undefined either way, but the property is genuinely absent when there is no close time. Verified against an event with an empty ledgerClosedAt — the key is absent from the returned object rather than set to undefined.
| } | ||
| case 'migrate': { | ||
| const tuple = xdr.ScVal.fromXDR(valueXdr, 'base64'); | ||
| const items = tuple.obj()?.vec(); |
There was a problem hiding this comment.
ScVal has no .obj() method in this SDK version, confirmed by tsc. Same issue at line 376. Use the accessor the installed SDK actually exposes for reading a vec/address out of an ScVal (e.g. .vec() / .address() directly, matching the pattern the rest of this file already uses for parseScValAddress).
There was a problem hiding this comment.
Fixed at both sites. ScVal in 14.6.1 exposes the accessors directly, so parseScValAddress now reads the switch and delegates to Address.fromScVal — the same decoding decodeScVal already uses elsewhere in this file:
function parseScValAddress(val: xdr.ScVal): string | null {
if (val.switch().name !== "scvAddress") return null;
try {
return Address.fromScVal(val).toString();
} catch {
return null;
}
}The migrate arm reads .vec() directly instead of .obj()?.vec(). Both it and the paused arm (.b()) are now guarded by a switch().name check first, because reading a mismatched arm on an xdr.ScVal throws at runtime — a malformed or unexpected event is skipped rather than crashing the whole read.
Since getEvents returns already-decoded ScVals, parseAddressXdr and its base64 round-trip are gone as dead code.
|
@collinsezedike thanks for the review — all four findings are fixed and I've replied inline to each thread. Head is now Summary of what changed in
Fixing #1 had a knock-on effect worth calling out: One behavioural change beyond the four items, flagging it explicitly in case you'd rather I revert it: the Verification:
The diff is limited to this one file (93 insertions, 69 deletions). |
|
@collinsezedike Thanks for the detailed review! I have addressed all the requested changes:
GitHub reports this PR as mergeable: true (no conflicts). Could you re-check? If a specific file still conflicts on your end, let me know and I will rebase immediately. |
b66e84d to
84df9cd
Compare
84df9cd to
7efe87b
Compare
|
Thank you for the contribution, @ZacLou. Verified: all required checks are green, and the RPC-based admin-history reader is additive to the existing Horizon-based one (issue #616), not a duplicate. Fixed three mechanical issues along the way: cargo fmt on the vault contract, prettier formatting on the new test file, and added coverage for the RPC reader's event-parsing branches (paused/transfer/accept/adapter/migrate, plus the unrecognised-action and short-vec skip paths) since the package's coverage thresholds require it. If you have a moment, a star on the repo would be appreciated. Merging now. |
Closes #697
Adds explicit event emission to the vault contract's admin functions and an RPC-based reader so the admin action history no longer requires scanning Horizon's global operation feed.
Contract changes:
set_pausedemits(admin, "paused", paused)transfer_adminemits(admin, "transfer", new_admin)accept_adminemits(admin, "accept", new_admin)set_adapteremits(admin, "adapter", new_adapter)migrate_adapteremits(admin, "migrate", (old_adapter, new_adapter))SDK changes:
packages/stellar-sdk-helpers/src/admin-history.tsuses RPCgetEventsfiltered by vault contract ID and the top-leveladmintopic, returning typedAdminAction[].packages/stellar-sdk-helpers/src/index.ts.