Repository navigation
feat(scenarios): keep-alive for every account of a kind, and per-override stop and restart - #53
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
What it does: adds OverrideInstance.keep_alive (keepAlive in JSON, defaults to true). An override with keepAlive: false writes its account once and does not enroll it in keep-alive.
Assessment: the change is small and correct.
- The gate in
apply_overridessits inside thescheduledcheck, so non-scheduled keep-alive rewrites are unaffected. - The serde default keeps existing payloads (scenarios, Studio, MCP) working as today.
OverrideInstance::newsets the field totrue, which matches the serde default.
Issues:
- No behavior test. The only test checks serde parsing. Nothing in this repo checks that
keepAlive: falseactually skipsenroll_keep_alive. A small svm-level test would catch a regression in the gate: register a test template with akeep_aliveimport, apply a scheduled override withkeep_alive: false, and assertkeep_alive_bindingsstays empty. The PR body says the behavior was checked on another branch, but that isn't covered by CI here. - Opt-out does not undo an earlier enrollment. If an earlier override already enrolled the account, a later
keepAlive: falseoverride on it has no effect. The PR describes this, but users can easily misread it. Consider logging atdebug!/info!when an opted-out override targets an account that is already inkeep_alive_bindings, or mentioning it in the field doc. - Unrelated file in the diff. The diff includes
.github/workflows/claude-review.yml(141 lines), which has nothing to do with this feature. If the file isn't meant to be part of this PR, it should go in a separate PR.
| // if no scenario had touched it. | ||
| let mut keep_alive_source = None; | ||
| if scheduled | ||
| && override_instance.keep_alive |
There was a problem hiding this comment.
The gate is correct, but only the serde test covers this PR. Consider an svm-level test: a scheduled override with keep_alive: false on a template that has a keep_alive import should leave keep_alive_bindings empty, and the same override with the default should add a binding. That would catch a regression in this condition.
| #[serde(default)] | ||
| #[cfg_attr(feature = "ts-bindings", ts(as = "Option<bool>", optional))] | ||
| pub fetch_before_use: bool, | ||
| /// Whether keep-alive rewrites the account after this override, when its template imports |
There was a problem hiding this comment.
Consider stating here (and in the schemars description) that false only skips enrollment. If an earlier override already enrolled the account, it stays kept alive. Users will likely read keepAlive: false as "stop keeping this account alive". A debug! in apply_overrides for an opted-out override whose account is already bound would also help debugging.
| /// Whether keep-alive rewrites the account after this override, when its template imports | ||
| /// keep-alive. Off, the override writes once and its fields age as the clock moves on. | ||
| #[schemars( | ||
| description = "If false, the override writes once and keep-alive does not keep the account fresh afterwards. Defaults to true" |
There was a problem hiding this comment.
Nit: the description says "Defaults to true" in prose. fetch_before_use sets its default with plain #[serde(default)]. If the generated JSON schema should advertise the default, add #[schemars(default = "default_keep_alive")] so MCP clients see it in the schema itself.
…r restart keep-alive per account
92Infinitus92
changed the title
There was a problem hiding this comment.
Summary
The PR adds a per-override keepAlive flag (default true). Serde, the TS types, rpc_endpoints.json and the README are all updated for it.
Scope and description mismatch
The PR does more than the title and body say:
-
Commit
bafeba3adds a new feature that the description never mentions: theownedkeep-alive address type,KeepAliveKind,account_discriminator,YamlOwnersand an IDL check incheck_keep_alive_imports. This is a separate feature and deserves its own description, or its own PR. -
The CI workflow is unrelated:
.github/workflows/claude-review.ymlshould not ship in this PR. -
The description contradicts the code. The body says "The flag only decides whether this override enrolls its account. An account that another override already enrolled stays kept alive." The code does the opposite:
keepAlive: falsedrops every existing binding for that address.- It also adds the address to
excluded, which blocks future enrollment from other overrides and filters it out ofownedkinds.
Either the description or the code is wrong. The README and tests match the code, so the description is probably out of date.
Main concerns
- Re-enabling does not restore input/PDA bindings. See the inline comment on
keep_alive.rs:66. - Per-slot cost of
ownedkinds. Every slot,get_account_owned_byclones every account of each owner. If one read fails, the whole owner is skipped for that slot.
Tests cover serde defaults, exclusion and re-inclusion through owned kinds, and the "failed write excludes nothing" path. None of them shows that a plain input binding comes back after re-enable, which is the gap described above.
| for source in touched { | ||
| if !source.keep_alive { | ||
| self.bindings | ||
| .retain(|binding| binding.address != source.address); |
There was a problem hiding this comment.
Re-enabling does not restore dropped bindings. retain permanently drops every binding whose address is this account, including bindings created by a different importing template (for example, the vault's input: oracle import).
A later keepAlive: true override on the same account only removes it from excluded (line 72). It does not restore those bindings. Keep-alive resumes only if:
- the original importing override (the vault) plays again, or
- an
ownedkind happens to cover the account.
The test at line 389 only passes because oracle is reached through the owned kind, not through its binding.
This contradicts the README ("A later override with keepAlive: true … on that account starts it again"). Two possible fixes:
- keep the bindings and filter
excludedintargets(), as is already done for owned kinds; or - make the docs say that re-enabling needs the importing override to replay.
Add a test that uses an input binding only, with no owned kind.
| or its owner with its owned accounts, is marked offline. Keep-alive is kept in memory only: after a restart on a persistent database it stops | ||
| never takes effect. An override with `keepAlive: false` stops keep-alive for the account it writes | ||
| from that slot on, so the account ages as the clock moves on, for example to test a stale price. A | ||
| later override with `keepAlive: true`, the default, on that account starts it again. An account not |
There was a problem hiding this comment.
This sentence is inaccurate for input/PDA bindings, as explained on keep_alive.rs:66. Re-enabling only clears the exclusion. A binding that was dropped is not recreated unless the importing override plays again.
The PR description also says the opposite: that an account another override already enrolled stays kept alive. Please align the description, the docs and the code.
| unix_time_ms: self.updated_at, | ||
| }; | ||
| let targets = self.keep_alive.targets(template_registry(), |owner| { | ||
| self.get_account_owned_by(owner).unwrap_or_else(|e| { |
There was a problem hiding this comment.
Per-slot cost and error handling for owned kinds.
This runs every slot for each owner of each owned kind. get_account_owned_by clones every account (full data) of the owner just to check an 8-byte prefix. keep_alive_overrides then calls get_account on each target again. On a busy fork, for example the Pyth receiver program with many price accounts loaded, this adds a full scan of that owner's accounts to every slot.
Also, get_account_owned_by collects into a Result. If any single account read fails, keep-alive silently skips all of that owner's accounts for the slot.
Suggested fix: iterate accounts_by_owner directly and check the discriminator on borrowed data, skipping only the accounts that fail to read.
…eepAlive is on again
There was a problem hiding this comment.
Summary
The PR adds an owned keep-alive address kind, which rewrites every local account of the given owners that carries an IDL account discriminator. It also adds a per-override keepAlive flag that stops and restarts keep-alive for one account. The per-slot logic now lives in KeepAlive::enroll and KeepAlive::targets, so it can be tested without the bundled registry. That is a good refactor, and the tests cover the stop and restart cases well.
Assessment
The design is sound. The points below are mostly about edge-case semantics and per-slot cost:
- Unrelated CI file. The PR also adds
.github/workflows/claude-review.ymlin commite15c9a0. It has nothing to do with scenarios and would be better as its own PR. - Per-slot cost of owned kinds. Every slot,
targetscallsget_account_owned_byfor each owner of each kind. That clones every account the owner has locally, including its full data, only to check an 8-byte prefix.keep_alive_overridesthen reads each matching account again throughget_account. If two kinds or imports share an owner, the owner is scanned twice. On a fork that has loaded many accounts of one program, this runs on every slot. Some fixes: cache results per owner within onetargetscall, have the lookup closure return only matching pubkeys, or skip the secondget_accountfor owned targets, since they are known to be local. - Exclusion is per address, not per import.
keepAlive: falseon an account stops every keep-alive template that writes it. That matches the docs. It also means one override can't keep one template, such as freshness, while stopping another. That's fine, but it's worth one sentence in the README.
| /// An import that rewrites a stopped account is bound too, and starts when the account does. | ||
| pub fn enroll(&mut self, registry: &TemplateRegistry, touched: Vec<KeepAliveSource>) { | ||
| for source in touched { | ||
| if !source.keep_alive { |
There was a problem hiding this comment.
When keep_alive is false, this continues before the source template's own imports are bound, and it excludes source.address itself. For an importing override, such as a vault with keepAlive: false, that means two things:
- The vault is excluded, but keep-alive never writes it anyway.
- Its oracle imports are never bound, so a later
keepAlive: trueon the oracle restarts nothing for this vault.
That clashes with the "off first" behaviour in the docs, where bindings are kept so a restart covers them. It would be more consistent to bind the imports regardless and only add the address to excluded. Otherwise, document that keepAlive: false on an importer also skips its imports.
| for binding in &self.bindings { | ||
| if let Some(import) = import_of(&binding.template, binding.import) | ||
| && !self.excluded.contains(&binding.address) | ||
| && seen.insert((import.template.as_str(), binding.address)) |
There was a problem hiding this comment.
Deduplication is keyed by the keep-alive template and address only. Two imports can use the same keep-alive template with different values on one account, for example posted_slot: $slot and posted_slot: $slot-5, or different field sets. In that case the first one in BTreeSet order wins and the other is dropped silently. Consider keying by the import, meaning (template, index), or logging when a conflicting import is dropped.
| continue; | ||
| }; | ||
| for owner in owners.iter().filter_map(|owner| owner.parse().ok()) { | ||
| for (address, owned) in accounts_owned_by(&owner) { |
There was a problem hiding this comment.
This scans each owner once per kind, and the closure clones every owned account, including its full data, every slot. If several kinds or imports name the same owner, the scan repeats. Cache accounts_owned_by(owner) per call, or change the closure to take the discriminator and return only matching pubkeys, so it doesn't clone data it never uses.
| }; | ||
| let missing = match self.get_account(&binding.address) { | ||
| for (import, address) in targets { | ||
| let missing = match self.get_account(&address) { |
There was a problem hiding this comment.
Owned targets come from get_account_owned_by, so they are already known to be local. This get_account reads each one a second time every slot. Passing the account, or a local flag, out of targets would avoid the duplicate lookup.
There was a problem hiding this comment.
Summary
The PR adds an owned keep-alive address kind, which rewrites every local account of an owner/IDL account type each slot. It also adds a per-override keepAlive flag that stops keep-alive for an account and later starts it again. The per-slot logic moves into KeepAlive::enroll and KeepAlive::targets, which can be tested without the bundled registry.
The design is clean and well tested:
- A failed write excludes nothing.
- Bindings are kept while an account is excluded, so turning keep-alive back on restores imports from other templates.
- An account bound by two imports of the same keep-alive template is written once per slot.
check_keep_alive_importschecks that the account exists in the keep-alive IDL.reset_networkclears everything.
Points to consider
- Per-slot cost of owned kinds.
targetscallsget_account_owned_byfor each owner every slot, which clones every account that owner owns.keep_alive_overridesthen re-reads each matching account and builds an IDL-based override for it. If a client has pulled a large program's accounts (for example viagetProgramAccountson the Pyth receiver), this can add up to thousands of clones and rewrites per slot. Possible mitigations: filter on the discriminator before cloning, or cap or log the number of owned targets. - One bad account blanks a whole owner.
get_account_owned_bycollectsResults, so one unreadable account makes it return an error. The closure then turns that intoVec::new(), and every account of that owner is skipped that slot, not just the broken one. - Exclusion is per account, not per template.
keepAlive: falseon an oracle stops every keep-alive template that writes that address. That is what the docs describe, but it's worth stating explicitly in the README. - Unrelated file.
.github/workflows/claude-review.ymlis part of this diff (commit e15c9a0) and has nothing to do with the feature. Consider splitting it out.
| continue; | ||
| }; | ||
| for owner in owners.iter().filter_map(|owner| owner.parse().ok()) { | ||
| for (address, owned) in accounts_owned_by(&owner) { |
There was a problem hiding this comment.
This runs every slot and asks for every local account of each owner, fully cloned, before filtering on the discriminator. With many accounts under a program like the Pyth receiver, that is a lot of cloning plus one IDL rewrite per match on every slot. Could the lookup return only the pubkeys and the 8-byte prefix, or filter inside get_account_owned_by? A cap or a warning when an owned kind reaches more than N accounts would also help.
| unix_time_ms: self.updated_at, | ||
| }; | ||
| let targets = self.keep_alive.targets(template_registry(), |owner| { | ||
| self.get_account_owned_by(owner).unwrap_or_else(|e| { |
There was a problem hiding this comment.
get_account_owned_by collects into Result<Vec<_>>, so one unreadable account makes it return an error. This closure then turns the error into Vec::new(), and every account of that owner misses keep-alive for the slot. Consider skipping only the account that fails, for example with a filter_map that logs the error.
| @@ -609,7 +609,7 @@ pub struct SurfnetSvm { | |||
| pub scheduled_overrides: Box<dyn Storage<u64, Vec<OverrideInstance>>>, | |||
| /// The accounts keep-alive rewrites every slot, from the slot a scenario's override first | |||
There was a problem hiding this comment.
This doc comment is out of date now that the field also holds owned kinds and the excluded set. It should say the field covers bindings, owned kinds and accounts excluded by keepAlive: false.
| } | ||
| continue; | ||
| } | ||
| self.excluded.remove(&source.address); |
There was a problem hiding this comment.
This removes the exclusion before the template lookup, so an override with keepAlive: true on an unknown or import-less template still restarts the account. That is intentional and covered by tests. A short comment here saying the restart does not depend on the template having imports would stop someone from moving it below the registry.get early-continue later.
Keep-alive rewrites only the accounts an import names. A transaction often reads accounts no scenario names: a Project 0 or Save liquidation reads every price feed of the borrower, a Jupiter Perps deposit or swap reads every feed of the JLP pool. Those go stale and the transaction fails. A scenario also had no way to keep a moved price from being kept alive, or to stop keep-alive later.
ownedaddress kind.address: { type: owned, owner: <program or list>, account: <IDL account> }rewrites every local account of those owners whose data starts with that account's discriminator, every slot, including accounts loaded after the override played. It uses theaccounts_by_ownerindex and never fetches. The keep-alive template needs an IDL with that account;check_keep_alive_importsrefuses it otherwise.keepAliveper override (OverrideInstance.keep_alive, defaulttrue). An override withkeepAlive: falsestops keep-alive for the account it writes from its slot on: its bindings go and owned kinds skip it, so it ages as the clock moves on, for example to test a stale price. A later override withkeepAlive: trueon that account starts it again.KeepAlive::enrollandKeepAlive::targetsinscenarios/keep_alive.rs, which take the registry and an account lookup, so they are tested without the bundled registry.reset_networkclears bindings, kinds and exclusions.rpc_endpoints.jsonand the generated kit types describe the new kind and field.Tests: types (owned with one owner or a list, bad or empty owners), registry (owned needs its account in the keep-alive IDL), keep-alive (owner and discriminator filter, an account loaded later, written once with a binding; stop and restart), and an svm test that a failed write excludes nothing,
falseexcludes andtruerestores. They fail without the change. Core suite 891/0 withignore_tests_ci, types 104/0, fmt clean, no clippy findings on changed lines.