Repository navigation
Conversation
How to use the Graphite Merge QueueAdd the label add-to-gt-merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
cae3e67 to
3698e79
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. 4 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 6 reviews per hour. WalkthroughOn-chain fills now use the wrapper ratio at the confirmed fill block to calculate underlying-share quantities and prices. Position events retain wrapped quantities and ratio evidence. Processing records zero-underlying fills as skipped and acknowledged, and resumes durable fills without rereading historical ratios. The P&L ledger stores ratio evidence and reports when legacy fills lack it. Inventory accounting uses wrapped quantities, and the dashboard labels on-chain quantities as wrapped shares. Priority: ➖ Normal Merge Risk: 🔵 Low · up to The wrapped-fill change looks safe in the reviewed P&L warning, spec and test edits. One earlier concern remains open: if recording a skipped fill fails, the fill may still be acknowledged and the reconciliation record lost. Owners should confirm or fix this before or soon after merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 76.83% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 164 functions across 24 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@rain-marvin review |
|
🔎 Reviewing |
![rain-marvin[bot]](https://avatars.githubusercontent.com/in/5077017?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Opus 5.5
This PR converts Raindex fills of ERC-4626 wrapped equities (for example SGOV) into underlying broker-share units before Position accounting and hedging. It reads the wrapper ratio at the fill block, keeps notional fixed, stores the ratio on OnChainFillApplied, and carries it into the P&L ledger. P&L marks symbols as unavailable when they have legacy fills with no recorded ratio.
The hedge-sizing fix is sound, and the fail-closed ratio read is the right idea. But the new underlying-unit amount also flows into consumers that still work in wrapped units, mainly the onchain inventory reactor and the trading-disabled cover instruction. The legacy P&L exclusion is also broader than SPEC.md describes: it hides every symbol traded before deploy, for every date range, with no path to recover, and the portfolio return is still computed from the partial result. I checked each finding below against the code at 3698e79.
ac3039a to
05512af
Compare
|
@coderabbitai review |
|
@rain-marvin review |
|
🔎 Reviewing |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Claude Opus 5.5 (Codex 1)
This PR converts Raindex fills of ERC-4626 wrapped equities (for example SGOV) into underlying broker-share units before position accounting and hedging. It reads the wrapper ratio at the fill block, keeps the cash notional the same, and stores the ratio with the fill. P&L marks a symbol unavailable when it has legacy fills with no recorded ratio, unless a later zero manual adjustment proves that the position was flat.
At 05512af the fixes for the five earlier threads are all in place. The inventory slot stays in wrapped shares, the legacy cutover works, capital return is suppressed, the cover and process-tx outputs use broker units, and the accountant witnesses the fill before it reads the ratio. The hedge-sizing path is sound. The panel found nothing that blocks. Three minor items remain. The main one is that process-tx still reads the historical ratio before it witnesses and deduplicates the fill, so it does not match the accountant fix. The other two are the simulation fixture and the leftover legacy residual on existing positions.
Two other items were checked and left out. A legacy fill after toDate can mark an earlier range unavailable, but that range is pre-deploy and the check fails closed. convertToAssets(1e18) scaling can round down by dust.
Panel note: the Cursor Grok 4.6, composer 2.5, and flash 3.7 lanes could not run on this host because their CLIs are not authenticated. The review ran on opus 5.5 (general and deep lanes) and sol 5.6 (general and special lanes), which meets quorum.
05512af to
6d8adcd
Compare
|
@coderabbitai review |
|
@rain-marvin review |
|
🔎 Reviewing |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Claude Opus 5.5 (Cursor 1)
This PR converts fills of yield-bearing wrapped equities (for example SGOV) from wrapped shares into broker shares before position accounting and hedging, using the wrapper's convertToAssets ratio at the fill block. It keeps the raw wrapped fill for audit and in the onchain inventory slot, stores the ratio as evidence for P&L, and marks symbols with unproven legacy basis as unavailable until a manual zero adjustment proves they were flat.
Overall read: at 6d8adcd every earlier thread is fixed or accepted with a documented deploy step. The panel traced every consumer of the fill events (hedging, inventory reactor, P&L ledger and replay, performance projection, trading-disabled cover detail, process-tx) and found the units consistent. No blocking defects. Three minor items are below: a legacy fill after toDate wrongly marks an earlier range unavailable, the dashboard banner has no guard for version skew, and the ratio is read from end-of-block state.
A design note, not posted as a finding: storing the ratio on OnChainOrderFilled next to wrapped_amount (with a serde default), and not on OnChainFillApplied, would remove the ledger UPDATE, the second column and index, the CASE in the snapshot loader, and the MissingOnchainFillBasisTarget error that can stop ledger ingestion. It is worth considering before this schema becomes permanent.
Panel note: the Cursor Grok, composer, and flash lanes could not run because their CLIs were not authenticated. The review met quorum with the opus and sol lanes (two general lanes and three specialist lanes).
6d8adcd to
9981d91
Compare
|
@rain-marvin review |
✅ Action performedReview finished.
|
|
@rain-marvin approve |
|
🔎 Verifying that my findings from |
There was a problem hiding this comment.
Claude Opus 5.5 (Claude 1)
This PR converts Raindex fills of ERC-4626 wrapped equities (for example SGOV) into underlying broker-share units before Position accounting and hedging. Onchain inventory stays in wrapped shares, each fill stores its proven fill-block ratio, and P&L marks legacy symbols unavailable until a zero manual adjustment proves the position was flat.
The review at a110b1b had no open findings. Since then, one commit (acdd113) addresses the three threads raised after it. I checked each one in the code at acdd113:
- Zero wrapper ratio escalates to a process-wide stop (4137846066): resolved.
normalized_fill_economicsnow returnsZeroUnderlyingAmountfor a zero ratio beforeUnderlyingPerWrapped::new.skip_zero_underlying_fillruns before the trading-disabled branch in both the worker andprocess-tx, so the fill is recorded and acknowledged and does not fail on every retry.zero_wrapper_ratio_fill_is_skipped_and_acknowledgedcovers it. - Unpriced-adjustment check ran after the cutover was chosen (4137544344, resolved by the author): addressed. The check now runs inside the candidate loop, next to the crossing check, and only looks at rows that replay after the candidate. It also follows the replay's price rules: only an onchain fill or a priced nonzero adjustment sets a price, a zero adjustment is skipped, and offchain rows are ignored. That matches
apply_manual_position_adjustmentandapply_onchain_fillin replay.rs. The L, A0, unpriced A1, A2 test asserts that A2 is chosen. - Cutover warning did not name the blocking row (4137846084): resolved. Both checks now return the blocking rowid. The warning names it and says to hedge the real exposure at the broker before recording the new zero adjustment.
Threads resolved by hand, not by me:
- 4137136443 (a
ZeroUnderlyingAmountrecord did not replace a decode-time row): addressed.record_skipped_fillupserts over an earlier row with a different reason, keeps the old reason, event type, time and detail, and never replacestrading_disabled. - 4137136450 (an unpriced adjustment after a cutover failed the whole report): addressed. It now marks only that symbol unavailable and moves on to the next candidate, as described above.
- 4128201006 (cutover replay-order crossings): case 1 addressed by
replay_order_crosses_cutover, which checks both directions. Case 2 dismissed, and the dismissal holds. It is an existing ordering issue between adjustments and fills inordered_position_eventson master, it does not depend on wrapped basis, and it is going to a separate follow-up. - CodeRabbit threads 4127325782 (replay order for the cutover), 4127649472 (stale SPEC listings), 4136593714 (propagate a failed skip-record write) and 4136954925 (resolve pending broker hedges before the manual cover): addressed. The code, SPEC.md and the PR description's deploy step match the fixes described in each thread.
I found no new defects in the a110b1b..acdd113 diff. The deploy step (pause orders, resolve broker hedges, flatten each legacy symbol with a zero adjustment) is in both SPEC.md and the PR description. When I checked, the backend build CI job was still running; every other check had passed.
|
@rain-marvin approve |
|
🔎 Verifying that my findings from |
There was a problem hiding this comment.
Claude Opus 5.5 (Claude 1)
This PR converts Raindex fills of ERC-4626 wrapped equities into underlying broker-share units before Position accounting and hedging. Onchain inventory stays in wrapped shares, and each fill stores its proven ratio. In P&L, legacy symbols stay unavailable until a zero manual adjustment proves the position was flat.
Since a110b1b there is one commit, acdd113. It makes two changes:
normalized_fill_economicsnow maps a zero wrapper ratio toZeroUnderlyingAmount. A fill on a drained wrapper now takes the per-fill skip, before the trading-disabled path, in both the worker andprocess-tx. It no longer fails as a process-scopedRatioerror on every retry.- The P&L unpriced-adjustment guard now runs inside the cutover candidate loop and follows the replay's price rules. A rejected candidate falls through to the next zero adjustment. The rejection warning names the blocking row and says to hedge at the broker before the new zero adjustment.
I checked that the guard scans only rows after the candidate, which matches the retain step (it keeps rows at or after the cutover). Only an onchain fill or a priced nonzero adjustment counts as a price, the same as replay.rs. CI is green at this head. I found no new defects in this diff.
Earlier findings:
- The two gaps noted in the last review (the guard not matching the replay's price rules, and the guard not trying a later cutover): resolved in acdd113, as described above, with tests
priced_zero_cutover_does_not_price_a_later_unpriced_adjustmentandunpriced_adjustment_rejects_a_cutover_and_a_later_zero_adjustment_recovers. - Zero ratio escalating as a process-scoped error: resolved. The ratio is checked before
UnderlyingPerWrapped::new, andzero_wrapper_ratio_fill_is_skipped_and_acknowledgedtests it. - Warning text not naming the blocking row: resolved.
Threads resolved by hand:
- 4128201006 (cutover crossing cases): addressed in part. The inverse crossing is fixed and tested. The dismissal of case 2 holds: a fill racing a later manual adjustment already mis-replays on master for every symbol, whatever its wrapped basis. It should get its own follow-up issue.
- 4137136443 (a zero-underlying skip hidden behind an earlier decode-time row): addressed.
record_skipped_fillletsZeroUnderlyingAmountreplace a decode-time row and keeps its detail. It never replacestrading_disabled, and a test covers both cases. - 4137136450 (unpriced adjustment after the cutover failing the whole report): addressed. Only that symbol becomes unavailable. Not carrying the dropped legacy price is correct, because that price has unknown basis.
- 4137544344 (unpriced check not trying a later cutover): addressed in the candidate loop, with a test.
No open findings remain.
acdd113 to
a03d211
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
a03d211 to
740ede7
Compare
|
Heads up for reviewers: in 740ede7 I changed how P&L treats fills recorded before this PR, per Juan's call. They keep counting 1:1 in wrapped shares, as they do on master, and the report warns when a range includes them. I dropped the "unavailable until a zero-adjustment cutover" approach and everything around it: the cutover candidate rules, capital suppression, and the dashboard banner. Why: the old approach blanked all P&L history for every symbol to avoid SGOV's pre-merge error, which is about 1%. For every ratio-1 wrapper, the 1:1 figures were already exact. New fills still store their ratio and are exact. Hedging is unchanged. The per-fill ratio storage and ledger migration stay. The deploy no longer needs a flatten, so the rollout in the description is now "deploy after close". The SGOV residual fix (about 15.7 shares short) is optional. |
|
@coderabbitai review |
|
@rain-marvin review |
|
🔎 Reviewing |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Claude Opus 5.5 (Claude 1)
This PR converts each wrapped (ERC-4626) Raindex fill into underlying broker shares, at the wrapper ratio of the fill's block, before it updates the Position and hedges. Onchain inventory stays in wrapped shares, the ratio is stored with each fill, a fill that converts to zero underlying shares is skipped per fill, and process-tx shares the same path.
Since the last review (acdd113), the branch was rebased onto the current master and the P&L design changed: the zero-adjustment cutover and the unavailable-symbol handling are gone. Fills recorded before this change now count 1:1 in wrapped shares, with a warning. That is exact for every ratio-1 wrapper and about 1% off for pre-merge SGOV, as the description says.
I traced the hedge, Position, inventory, cover-instruction and process-tx paths again at 740ede7. All of them use the right units, and the fixes from the earlier threads are still in place after the rebase. No blocking issue. Two minor points: the new legacy warning shows on every report for good, and no test covers a legacy lot matched against a new fill at a ratio other than 1.
The panel ran with two models. The other lanes could not authenticate and were dropped.
740ede7 to
ab5eba9
Compare
|
@coderabbitai review |
|
@rain-marvin approve |
✅ Action performedReview finished.
|
|
🔎 Verifying that my findings from |
There was a problem hiding this comment.
Claude Opus 5.5 (Claude 1)
This PR converts Raindex fills of ERC-4626 wrapped equities (for example SGOV) into underlying broker-share units before Position accounting and hedging. Onchain inventory stays in wrapped shares, and each fill stores its proven ratio.
The branch was rebased onto master (#1543, #1557, #1576, #1573). I compared the PR's own patch before and after the rebase. The shared files (SPEC.md, src/api.rs, src/operator.rs, src/rebalancing/trigger/mod.rs) only changed in SPEC.md, where the text now follows the new P&L design.
The main change since a110b1b is a simpler P&L design. The zero-adjustment cutover, the per-symbol "unavailable" state, and the capital suppression are all gone. Legacy fills that have no ratio now count 1:1 in wrapped shares, as before, and the report warns only when the queried range holds such a fill for a symbol whose recorded ratios are not all exactly 1. The warning compares against RATIO_ONE.to_string(), and that matches how the ledger stores the ratio (U256 in decimal). The dashboard, the response DTO, and the capital path no longer refer to unavailable symbols anywhere. This removes a lot of edge-case code that my earlier threads kept finding problems in. The remaining legacy error is small, bounded by the ratio minus 1, and it affects only the P&L display. Hedging does not depend on it.
One more change: a zero wrapper ratio at the fill block now takes the per-fill ZeroUnderlyingAmount skip. Before, UnderlyingPerWrapped::new returned an error and the fill failed on every retry. A new test covers the skip and the acknowledgement.
Earlier findings:
- My last review (a110b1b) approved with no blocking findings. Its two minors are resolved.
- Thread 4139160365 (legacy-ratio warning on every report): resolved. It now fires only for a legacy fill inside the range, and skips symbols proven 1:1. Tests:
legacy_fill_outside_the_range_raises_no_warningandlegacy_fill_on_a_proven_one_to_one_wrapper_raises_no_warning. The reply leaves out the "legacy lot still open at range start" part, so a range that starts after a legacy SGOV buy and closes that lot gets a slightly wrong realized figure with no warning. The error is bounded and affects display only, and the main complaint (noise) is fixed, so I accept that. - Thread 4139160371 (no test for a legacy lot matched against a new fill at a ratio other than 1): resolved.
legacy_wrapped_lot_matches_a_new_underlying_fill_at_ratio_above_onepins realized -5, an open short of 0.1, no reconciliation note, and the SGOV warning.
Threads resolved by hand:
- 4128201006: addressed. Case 1 was fixed earlier. Case 2 is an ordering problem that already exists on master, split out as a follow-up. The cutover code is now removed.
- 4137136443: addressed.
ZeroUnderlyingAmountreplaces an earlier decode-time skip row and keeps its detail, but never replacestrading_disabled. A test covers both cases. - 4137136450 and 4137544344: no longer apply. The cutover and unpriced-adjustment logic they covered is removed, so a cutover can no longer fail the whole report on an unpriced adjustment.
No new defects in the delta. CI was still re-running at the time of this review. The failed run was a cancelled, superseded run, not a code failure.
Merge activity
|
github-actions
Bot
added
the
externally-merged
2 participants
The bot now converts each wrapped Raindex fill into underlying shares, at the ERC-4626 (tokenized vault) ratio of the fill's block, before it updates the Position and hedges. Today it assumes 1 wrapped share equals 1 broker share, so it under-hedges SGOV, and P&L counts the wrapper's yield as a trading loss. RAI-2666
Live effect: hedge sizes change for every wrapper whose ratio is not 1 (larger for SGOV). P&L keeps counting pre-deploy fills 1:1, so pre-merge SGOV P&L stays about 1% off and new fills are exact · Risk: high (money path, stored data; no keys or IAM) · Ships: on the next release, deployed after market close
Decisions
skipped_fillsunhedged, not retried. A retry cannot succeed and would trip the conductor-wide fail-stop. conductor.rsRisks
Proof
nix run .#prodVerifyMigrationson the first revision: the migration applies to a production snapshot and every stored event replays. The migration has not changed since, but the code has.convertToAssets. All 54 unwrapped tokens the old startup probe skipped in the prod and staging configs report 18 decimals. Nobody rehearsed the flatten or tested a rollback.Rollout
TradingDisabledrows of wrapped symbols at the wrapped quantity × the ratio at the fill's block, not the recorded figure.Wrapper ratio changed within the fill's blockwarnings, and newzero_underlying_amountrows.🤖 Generated with Claude Code