refactor(events): narrow the delta events' amount to uint120 - #54
Conversation
All four delta events declared a uint256 amount next to uint120 balances, so a consumer decoding the tuple saw two widths for three quantities of the same kind. No value changes. In capture and refund the emit now passes the uint120 local that ALREADY existed for the storage write (captureAmount120 / refundAmount120) instead of the uint256 parameter — so no new cast is introduced, and the two existing safe-cast comments still carry the justification. void and release were already emitting a uint120 local that only the event signature widened back. BREAKING: topic0 changes on all four. PaymentCaptured 0x460e11c3… → 0x77b933fc… PaymentVoided 0xe60803c7… → 0xb37b3ce4… PaymentReleased 0xaf7a769d… → 0x90204216… PaymentRefunded 0xaa05bfb9… → 0xe53d95a7… So this must NOT deploy alone. It needs a VERSION bump and the coordinated rollout, plus a full reindex. rail0#53 (the minimum refund window) is also breaking and also unmerged — one bump covers both, and shipping them together avoids two reindexes. VERSION is deliberately left at 1.3.0 here, as in #53: bumping it is a release decision across six live deployments. The companion indexer change is commercelayer/rail0-indexer#78. Gas: +18 on capture (178,347 → 178,365), from reading the local rather than the parameter. A uint120 still occupies a full word in event data, so nothing is saved there either — this is consistency, not optimisation. The four locally re-declared events in RAIL0.t.sol had to be narrowed too, and all four event tests failed until they were. That is the clearest local evidence topic0 moved, and it is noted in the test file so the next reader sees why they exist. Suite: 120 tests pass. forge fmt --check clean. Closes #47 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Gas, measured properly this timeThe PR body says "+18 on capture, from reading the local rather than the parameter". The number was right; the explanation was too broad. Full picture, measured against a clean
Deployed bytecode: 7,212 → 7,221 bytes (+9). Why it splits that wayThe two halves of the change have opposite gas profiles, which is the part the original note flattened:
An inline A methodology note, since it nearly published a wrong numberAn intermediate measurement gave a 178,212 baseline and an apparent +153. That was an artefact: reverting the change by hand left a mixed state (one emit reverted, the other not), and the optimizer's output shifted enough to move Verdict+18 and +21 on two of eight entrypoints, 0.01% each. Not a reason to reconsider the change, and not an optimisation either — the same conclusion as before, now with the numbers behind it. |
Closed — measured, this makes gas worse, and the defect is cosmeticRecording the decision here since the PR was closed without it.
Plus 9 bytes of bytecode. Narrowing an event's So the trade is 18–21 gas against ABI self-consistency, and #47 called the inconsistency cosmetic in its own title. Not worth a cost on two of the six fund-moving entrypoints. Worth contrasting with #55, where the transient-storage lock saves ~2,000 gas on all six: there gas and code quality pointed the same way; here they oppose and gas wins. Issue #47 is closed as won't-do with the same reasoning. commercelayer/rail0-indexer#78 is closed with it — without the contract change its golden signatures would be wrong. The branch stays, so the change is trivial to revive from this diff if the decision ever flips. |
All four delta events declared a
uint256 amountnext touint120balances, so a consumer decoding the tuple saw two widths for three quantities of the same kind.No value changes, and no new cast
In
captureandrefundthe emit now passes theuint120local that already existed for the storage write —captureAmount120/refundAmount120— rather than theuint256parameter. So no cast is introduced and the two existing safe-cast comments still carry the justification.voidandreleasewere already emitting auint120local that only the event signature widened back touint256, which is the clearest sign the declaration was what was off.BREAKING — topic0 changes on all four
Must not deploy alone. It needs a
VERSIONbump, the coordinated rollout, and a full reindex.Bundle it with #53 (the minimum refund window), which is also breaking and also unmerged: one bump covers both, and shipping them together avoids two reindexes.
VERSIONis deliberately left at1.3.0here, as in #53 — bumping it is a release decision across six live deployments, not a code change to slip in.Companion indexer change: commercelayer/rail0-indexer#78.
Gas
+18 on capture (178,347 → 178,365), from reading the local rather than the parameter. Note a
uint120still occupies a full word in event data, so nothing is saved there either — this is consistency, not optimisation. Reporting it rather than claiming "no change".What the tests proved
The four events are re-declared locally in
RAIL0.t.solforvm.expectEmit, and all four event tests failed until those declarations were narrowed too. That is the clearest local evidence topic0 moved — and it is now noted in the test file so the next reader sees what those declarations are actually guarding.The indexer's
config.events.test.tsthen caught the same thing across repos: it comparesconfig.yamlagainst the compiled contract ABI from the sibling checkout, and failed with the old selector until the contract was rebuilt. Worth knowing that guard works.Suite: 120 tests pass.
forge fmt --checkclean.Closes #47
🤖 Generated with Claude Code