test(tips): cover the S3 parser and key handling - #47
Conversation
app/api/tips/s3.ts had no tests: 317 lines doing JSON parsing, bigint coercion, and S3 key parsing, all of it silently returning null or [] on failure. That forgiveness is why placeholder credentials once presented as "no data" rather than a misconfiguration — nothing distinguished the two. Stubs only ./config, so every function runs its real body and the parsing is genuinely exercised rather than reimplemented in the test: - getBlockFromCache: bigint coercion of number/timestamp/gasUsed/gasLimit, the per-transaction gasLimit default of 0n, meterBundleResponse normalised to null, and null (not a throw) for malformed JSON or an uncoercible field. - listRejectedTransactions: the rejected/<block>/<hash> key format, skipping malformed keys, newest-block-first ordering, and MaxKeys. - getBundleHistory: prefix listing, and keeping the readable events when one is corrupt rather than dropping the bundle. - getTransactionMetadataByHash: key format and both null paths. - cacheBlockData: bigints serialise as strings and round-trip back through getBlockFromCache as bigints — the write and read halves agreeing is the only thing keeping the block cache usable. Error swallowing is pinned as current behaviour, not endorsed: an S3 failure still yields [] from listRejectedTransactions. Verified by mutation rather than a green run — dropping the gasLimit default, reversing the sort, removing the key-length guard, and skipping bigint serialisation each fail the suite.
🟡 Heimdall Review Status
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Checked the test fixtures against the code that actually writes these objects —
the audit archiver in base/base, crates/infra/audit/src/storage.rs — rather than
against our own TypeScript, and the two disagree.
TransactionMetadata declared `sender: string` and `nonce: string` as required.
The producer serializes `bundle_ids` and nothing else, so both fields were always
undefined at runtime while the type promised a string. Nothing reads them, in
this repo or in tips-ui, so this is a latent trap rather than a live bug: anyone
trusting the type would have got undefined with no compile error. The type now
matches the writer, and cites it.
Fixtures reworked to the real wire shapes:
- transactions/by_hash/<hash> carries only bundle_ids. Added cases for the UUID
form older objects use and for an object with no bundles.
- BundleHistoryEvent is #[serde(tag = "event", content = "data")], so events are
{ event, data } with per-variant payloads. The previous fixture had a bare
{ event } and would have passed even if the payload were dropped; the Received
and BuilderIncluded fixtures now carry real contents and are asserted, since
the block route reads data.bundle.meter_bundle_response.
S3 key formats confirmed against S3Key in the same file: transactions/by_hash/
{hash} and rejected/{block_number}/{tx_hash}.
| it('skips keys that are not exactly rejected/<block>/<hash>', async () => { | ||
| givenS3({ | ||
| listing: [ | ||
| 'rejected/100/0xa', | ||
| 'rejected/', // prefix marker | ||
| 'rejected/200', // missing hash | ||
| 'rejected/300/0xc/extra', // too deep | ||
| 'rejected/notanumber/0xd', // unparseable block | ||
| ], | ||
| }); |
There was a problem hiding this comment.
rejected view is going away soon w/ the new tx observability that Niran is rolling out
There was a problem hiding this comment.
Noted — same treatment as the bundle-history thread.
Kept for now since the rejected route is still live, but flagged with a note naming Niran's rollout as the replacement, on both this describe and formatRejectionReason (which goes away with the same view). s3.ts carries the matching note so it's visible from the code side.
That's 7 of the 24 tests in soon-dead paths. My reasoning for keeping rather than dropping them: they cost nothing to carry, they disappear with the code, and if the migration slips they're the only thing covering the rejected/{block}/{tx_hash} key parsing in the meantime. Easy to reverse if you'd prefer them gone now.
| }, | ||
| }); | ||
|
|
||
| const history = await s3.getBundleHistory('mainnet', 'b1'); |
There was a problem hiding this comment.
w/ the tx observability , we'll be moving towards Postgres for the full tx history and S3 for bundle history will be gone
There was a problem hiding this comment.
Good to know — thanks, I didn't have that context.
Left the tests in for now, since this is what production serves today and getBundleHistory currently feeds four routes, not just the bundle page: bundle/[hash], txn/[hash], the block/[hash] metering enrichment, and rejected. Until the Postgres path lands, these are the only coverage on that parsing.
What I did change is make the end date explicit, so they don't get mistaken for a long-term contract: a note on the describe block, and one in s3.ts itself listing exactly which functions the migration retires — someone editing the module won't necessarily open the test file. Both say to delete the tests in the same change that removes the S3 path.
Also not extending coverage here. The remaining API test work I had planned goes to what survives the migration (block/[hash] response serialization, route-level status contracts).
Happy to just delete them instead if you'd rather not carry them at all — say the word.
Review feedback on #47: transaction observability moves full transaction history to Postgres and retires S3-backed bundle history, and the rejected view is replaced along with it. Keeps the tests — this is what production serves today, and getBundleHistory still feeds four routes including block/[hash] metering enrichment — but records the end date in both places so they are not mistaken for long-term contracts and are deleted in the same change as the code. Noted in s3.ts too, since someone editing the module will not necessarily open the test file. No further coverage planned for these paths; remaining API test work goes to what survives the migration.
Per review: don't pin down code that is about to change. Removes the tests for getBundleHistory, getTransactionMetadataByHash, listRejectedTransactions, and formatRejectionReason — transaction and bundle history are moving to Postgres and the rejected view is being replaced, so that coverage would only have to be unpicked later. getTransactionMetadataByHash goes too, though it wasn't called out directly: it exists solely to resolve a transaction to its bundle_ids for getBundleHistory, so it moves with them. What's left is the block cache — getBlockFromCache and cacheBlockData — which is this app's own read-through cache of RPC block data and is untouched by the migration. Nine tests: bigint coercion, the per-transaction gasLimit default, null normalisation, the malformed and absent paths, and the write/read round trip that is the only thing asserting the two halves agree. Keeps the TransactionMetadata type correction from the previous commit. That is a production fix, not coverage: the type declared sender and nonce as required while the producer writes neither. 24 -> 9 tests here, 65 -> 50 overall. Mutation-checked that the remainder still bites.
|
Dropped the coverage on everything the observability work touches, rather than annotating it — pinning down code that's about to change just creates something to unpick later. Removed: Kept: the block cache — Also kept: the
24 → 9 tests here, 65 → 50 overall. Mutation-checked that what remains still fails when the code breaks. |
3 participants
First slice of API test coverage.
app/api/tips/s3.ts— 317 lines of JSON parsing, bigint coercion, and S3 key parsing — had no tests at all, and every failure path in it silently resolves tonullor[].That forgiveness has already cost us once: when the TIPS credentials were placeholders, every read returned
nulland the UI rendered—with no signal. Nothing distinguished "misconfigured" from "no data".Approach
Only
./configis stubbed (the S3 client and bucket name). Every function under test runs its real body, so the parsing is genuinely exercised rather than reimplemented in the test file. Commands are matched withinstanceofagainst the real SDK classes, so the assertions on keys and prefixes are checking what actually gets sent.What's covered — 22 tests
getBlockFromCache— bigint coercion ofnumber/timestamp/gasUsed/gasLimit; the per-transactiongasLimit ?? '0'default;meterBundleResponsenormalised tonull; returnsnullrather than throwing on malformed JSON or an uncoercible numeric field.listRejectedTransactions— therejected/<block>/<hash>key format, skipping malformed keys (prefix markers, missing hash, too deep, non-numeric block), newest-block-first ordering, andMaxKeys.getBundleHistory— prefix listing, and that one corrupt event doesn't discard the bundle's whole history.getTransactionMetadataByHash— key format and both null paths.cacheBlockData— bigints serialise as strings and round-trip back throughgetBlockFromCacheas bigints. The write and read halves agreeing is the only thing keeping the block cache usable, and nothing asserted it before.formatRejectionReason— all three branches.Error swallowing is pinned as current behaviour, not endorsed — an S3 failure still yields
[]. Worth revisiting separately (an outage and an empty bucket should not look identical), but changing it here would be a behaviour change hiding in a test PR.Verified by mutation, not by a green run
All 22 passed first try, which is not evidence of much. I mutated the source to confirm they bite:
?? '0'gasLimit defaultparts.length !== 3guardcacheBlockDataSource restored and verified identical afterwards.
Suite goes 41 → 63 tests.
typecheck,lint,test,buildall pass.Next
Tier 2 from the coverage review —
block/[hash]response serialisation (hex→BigInt, themeteringsplit), then route-level status-code contracts. Happy to follow up.