fix: cataloguing & assertions #6
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new volumes consistency check run per ledger, relocates and reworks assertion helpers (Details/With), refactors volume/balance validation into CheckVolume, adjusts transaction generation and overdraft logic, changes a bulk submit flag to non-atomic, and removes the old internal assert helpers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
workload/internal/random.go (1)
12-12: Consider making the RandomTimestamp time unit explicit
RandomTimestampcurrently applies an offset of up to 9 nanoseconds:offsetTime := presentTime.Add(time.Duration(-int64(random.GetRandom()%10)))If the intent was to jitter by a larger unit (e.g., seconds) rather than a few ns, consider multiplying by
time.Second(or another explicit unit) to avoid confusion:offsetTime := presentTime.Add(-time.Duration(random.GetRandom()%10) * time.Second)Semantics match previous behavior; this is just a clarity/intent improvement.
Also applies to: 45-51
workload/bin/cmds/parallel_driver_bulk/main.go (1)
65-71: Confirm non‑atomic bulk semantics are intendedSetting
Atomictopointer.For(false)changes the bulk submission from all‑or‑nothing to potentially partial application (depending on server semantics), whileContinueOnFailureis stillfalse.If the test’s intent is to exercise fully atomic bulks, this might be a behavior regression; if you’re deliberately stressing partial commits after
SubmitInvalidAtomicBulk, this looks correct. Please confirm that downstream assertions and expectations align with the non‑atomic behavior.workload/bin/cmds/parallel_driver_transactions/main.go (3)
101-112: Read‑after‑write assertion only treats NotFound as unreachableUsing:
if errors.As(err, &getTxError) { assert.AlwaysOrUnreachable( getTxError.ErrorCode != shared.V2ErrorsEnumNotFound, "should always be able to read committed postings transactions", ... ) }means only a 404/
NotFoundis considered “unreachable”; other typed errors are treated as acceptable from an assertion standpoint (thougherrmay still be non‑nil). If the intention is to specifically flag missing committed transactions while tolerating transient backend errors, this pattern is fine; if you also want to track non‑NotFound errors as assertion noise, you may want a separateassert.Sometimes(err == nil, ...)before theerrors.Asblock.
114-133: Per‑account overdraft handling: ensure coverage for accounts only in post‑commit volumes
initialOverdraftsis built fromPreCommitVolumesand then used to deriveallowedOverdraftfor each account (except"world"):initialOverdrafts := make(map[string]map[string]*big.Int) for account, volumes := range res.V2CreateTransactionResponse.Data.PreCommitVolumes { initialOverdrafts[account] = make(map[string]*big.Int) ... } for account, volumes := range res.V2CreateTransactionResponse.Data.PostCommitVolumes { var allowedOverdraft map[string]*big.Int if account != "world" { allowedOverdraft = initialOverdrafts[account] } internal.CheckVolumes(volumes, allowedOverdraft, ...) }This works well if every account present post‑commit also appears in
PreCommitVolumes(even with empty maps). If the API can ever return accounts only inPostCommitVolumes(e.g., accounts created by the transaction that have no prior volumes),initialOverdrafts[account]will benil, andCheckVolumeswill skip overdraft enforcement entirely for that account.If you want zero overdraft by default for such accounts, consider ensuring that all post‑commit accounts have an entry in
initialOverdrafts(possibly an empty map) before this loop, or defaultingallowedOverdraftto a non‑nil empty map instead ofnil.
175-193: Numscript read‑after‑write and post‑commit volume checks are consistentThe numscript read‑after‑write uses the same
AlwaysOrUnreachable/NotFound pattern as postings, and the subsequentCheckVolumes(volumes, nil, ...)correctly reuses the shared volume‑consistency helper without overdraft allowances for this path.This keeps the invariants checked in a single place (
internal.CheckVolumes+CheckVolume), which is a solid refactor.workload/bin/cmds/eventually_correct/main.go (1)
98-116: Never‑accounts query: confirm field name alignmentSwitching the
$matchfilter from"address": "never:"to"destination": "never:"and updating the message to"should be able to list never accounts"changes which field the query targets.If the API’s transaction schema indeed uses
destinationfor the account field being validated here, this is an important correctness fix. If not, it could silently stop matching the intended transactions. Please double‑check against the ledger’s filter semantics.workload/internal/utils.go (3)
20-31: Details.With is a useful helper; consider a value receiver for flexibility
Detailsasmap[string]anyplus:func (d *Details) With(extra Details) Details { out := make(map[string]any) for k, v := range *d { out[k] = v } for k, v := range extra { out[k] = v } return out }gives you a clean way to build enriched context without mutating the original map, which is good. Since the method doesn’t mutate
d, you could make the receiver a value:func (d Details) With(extra Details) Details { ... }This would avoid any chance of calling it on a nil
*Detailswhile keeping call sites the same. Not urgent, but slightly more idiomatic.
55-79: CreateLedger assertions and follow‑up GetLedger check mostly look goodThe new flow:
- Builds a base
detailsmap with"ledger": name.- Uses
assert.Sometimes(err == nil, ...)enriched with"error": errfor the create call.- Returns early on create error.
- Performs a
GetLedgerand, when the error is a typedV2ErrorResponse, treats onlyNotFoundas unreachable viaAlwaysOrUnreachable.This matches the pattern used elsewhere: treat 404s as invariants violations while allowing other server/network errors to be catalogued but not considered logically impossible.
If you’d like visibility into non‑NotFound
GetLedgerfailures as well (without marking them unreachable), you could optionally add a precedingassert.Sometimes(err == nil, ...)around the GetLedger call, similar to the create assertion.
110-126: GetPresentTime assertion could include the error in DetailsThe new
assert.SometimesinGetPresentTime:assert.Sometimes(err == nil, "should be able to get the latest transaction", Details{ "ledger": ledger, })is good for cataloguing failures, but it currently omits the actual
errvalue. Including it would make debugging easier and be consistent with other call sites:assert.Sometimes(err == nil, "should be able to get the latest transaction", Details{ "ledger": ledger, "error": err, })Control flow (
if err != nil { return nil, err }) is unchanged and correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
workload/go.modis excluded by!**/*.modworkload/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (10)
workload/bin/cmds/eventually_correct/main.go(3 hunks)workload/bin/cmds/eventually_correct/volumes.go(1 hunks)workload/bin/cmds/parallel_driver_bulk/main.go(1 hunks)workload/bin/cmds/parallel_driver_ledger_create/main.go(1 hunks)workload/bin/cmds/parallel_driver_transactions/main.go(5 hunks)workload/bin/init/main.go(1 hunks)workload/internal/assert.go(0 hunks)workload/internal/checks.go(1 hunks)workload/internal/random.go(2 hunks)workload/internal/utils.go(3 hunks)
💤 Files with no reviewable changes (1)
- workload/internal/assert.go
🧰 Additional context used
🧬 Code graph analysis (4)
workload/bin/cmds/eventually_correct/volumes.go (2)
workload/internal/utils.go (1)
Details(20-20)workload/internal/checks.go (1)
CheckVolume(32-41)
workload/bin/cmds/eventually_correct/main.go (1)
workload/internal/utils.go (1)
Details(20-20)
workload/bin/cmds/parallel_driver_transactions/main.go (3)
workload/internal/utils.go (1)
Details(20-20)workload/internal/checks.go (1)
CheckVolumes(11-30)workload/internal/random.go (2)
GetRandomAddress(21-23)RandomBigInt(14-19)
workload/internal/checks.go (1)
workload/internal/utils.go (1)
Details(20-20)
🔇 Additional comments (8)
workload/bin/init/main.go (1)
3-13: Import change is fine and behaviorally neutralReordering the imports to include
fmthere is correct and has no impact beyond style; usage infmt.Printfremains valid.workload/bin/cmds/parallel_driver_ledger_create/main.go (1)
13-19: No behavioral change in id generationThe
id := random.GetRandom()%1e6change is purely formatting; the modulo range and ledger naming behavior are unchanged.workload/bin/cmds/parallel_driver_transactions/main.go (2)
82-99: Postings transaction assertion pattern looks consistentThe new
assert.Sometimes(err == nil, ...)with contextualDetailsplus earlyif err != nil { return }cleanly separates assertion/logging from control flow. Details include ledger, postings, and error, which should be sufficient for debugging.
143-173: Numscript transaction changes look coherent with monetary amountsThe script and variables now align better with a monetary amount:
monetary $amountin the vars section.send $amount (...)in the script body.amountvar formatted asfmt.Sprintf("COIN %v", internal.RandomBigInt().String()).Assuming the ledger expects a
"ASSET amount"string for amonetaryvariable, this should be valid and clearer than a raw number. Theassert.Sometimes(err == nil, ...)+ early return mirrors the postings path and is consistent.workload/bin/cmds/eventually_correct/main.go (2)
30-41: Parallel volumes consistency check integrates cleanlyAdding
checkVolumesConsistent(ctx, client, ledger)into the per‑ledger goroutine continues the existing pattern of independent ledger checks under async.WaitGroup. The function is local to this package, uses only per‑call state, and should be safe to run alongside the other checks.
60-68: Balanced check message change is harmlessThe assertion now uses a static message (
"aggregated volumes for asset should be 0") while still attachingassetandvolumesinDetails. This keeps logs more structured without losing context.workload/internal/checks.go (2)
10-10: LGTM!The comment clarifies the conditional nature of overdraft checking and improves code readability.
11-30: LGTM on the refactoring approach!The extraction of balance validation logic into
CheckVolumeand the capitalization ofWithmethod improve code reusability and maintainability. The detail propagation is correctly handled.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 12 files
Prompt for AI agents (all 3 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="workload/internal/utils.go">
<violation number="1" location="workload/internal/utils.go:78">
`CreateLedger` ignores errors from the verification `GetLedger` call and still returns success, so callers never learn when verification fails.</violation>
</file>
<file name="workload/internal/random.go">
<violation number="1" location="workload/internal/random.go:12">
Drop the explicit semicolon so the constant declaration remains gofmt-compliant; otherwise formatting checks will continuously rewrite this line.</violation>
</file>
<file name="workload/internal/checks.go">
<violation number="1" location="workload/internal/checks.go:35">
`CheckVolume` no longer validates balances: it compares `balance` with itself, so mismatched volumes will silently pass. Compare against the computed `actualBalance` instead.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
workload/bin/cmds/eventually_correct/volumes.go (1)
17-80: Volumes consistency logic looks solid; consider hardening pagination metadataThe pagination and per-volume/account cross-checks look correct, and the error handling now avoids nil dereferences. As a small robustness improvement, you could assert that
volumes.V2VolumesWithBalanceCursorResponse.Cursor.Nextis non‑nil wheneverHasMoreis true before assigning it tocursor, so a malformed server response doesn’t send this loop into a tight retry on the first page.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
workload/bin/cmds/eventually_correct/volumes.go(1 hunks)workload/internal/checks.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
workload/bin/cmds/eventually_correct/volumes.go (2)
workload/internal/utils.go (1)
Details(20-20)workload/internal/checks.go (1)
CheckVolume(32-41)
workload/internal/checks.go (1)
workload/internal/utils.go (1)
Details(20-20)
🔇 Additional comments (1)
workload/internal/checks.go (1)
32-40:CheckVolumecorrectly compares reported balance to computed balanceComputing
actualBalancefrominputandoutputand assertingbalance.Cmp(actualBalance) == 0restores the intended invariant; the extra fields indetailsprovide good diagnostics.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
workload/internal/checks.go (1)
10-25: Overdraft bound check looks correct; minor detail/context improvements possibleThe overdraft logic and bound check via
Cmp(minimumBalance) != -1correctly enforcebalance >= minimumBalancegivenCmp’s{-1,0,1}contract. Two small, optional tweaks:
- Reuse the
overdraftlocal when populating details instead of re‑indexingallowedOverdraft[asset], to avoid divergence if the map changes between the two uses and to make the relationship clearer.- Consider including
minimumBalanceitself in the assertion details; when this trips, having both the configured overdraft and the derived minimum allowed balance would make failures easier to interpret.These are non‑blocking ergonomics improvements; the current behavior is sound.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
workload/internal/checks.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
workload/internal/checks.go (1)
workload/internal/utils.go (1)
Details(20-20)
🔇 Additional comments (1)
workload/internal/checks.go (1)
30-39: Balance consistency check is now correct; verify assumptions about nil volumesThe refactored
CheckVolumecorrectly computesactualBalance = input - outputand compares it to the reportedbalance, and the added fields indetails.With(...)should give good diagnostics when mismatches occur.One assumption to confirm: if any of
input,output, orbalancecan benilfrom upstream APIs, thebig.Intoperations will panic before hitting the assertion. If there’s any chance ofnilvalues, consider adding an upfront assertion (or defensive guard) that these pointers are non‑nil with details about the offending asset/account so failures are explicit rather than panics.
Summary by cubic
Adds a volumes consistency check and standardizes assertions across workloads to catch balance mismatches and improve reliability.
New Features
Refactors
Written for commit c445b5f. Summary will update automatically on new commits.