feat(scenarios): add an AMM swap scenario - #71
Conversation
The scenario declared 22460 gas for a mint. Measured against the deployed binding, a mint to a receiver holding none of the token needs 69319, and one to a receiver that already holds some needs 51757. Every mint the scenario sent landed in a block with a failed status, having burned the whole limit, and trackReceipts defaults to false so the run reported each one as sent. 22460 is ERC20Noop's constant, copied. PLT-1091 covers the two scenarios that still carry it. The limit is now 75000, and the test pins it against the measurement rather than against itself. Broke the constant back to 22460 and to 200000 on purpose; the test caught both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A DeFi profile had no contract to drive. This adds a constant-product pair with the storage and gas shape of a UniswapV2 swap: both reserves, the caller's balance in each token, and an event. The contract never reverts on bookkeeping, which is the choice StorageRWv1 already makes. The balances wrap rather than check, because nothing reads them back and a load generator that fails on its own accounting stops measuring the chain. A short caller is not credited: crediting exactly what is then debited returns the slot to zero, and a zero to non-zero storage write costs four times one that changes a slot already holding a value. Under the default mix, which draws one direction, that write would land on every swap rather than the first. The reserves sit between a floor and a ceiling. Without the ceiling the input side grows without bound and the output halves every 100000 swaps, so a long run prices nothing like its start. The ceiling is also what keeps one oversized call from ending the pair: a swap of 1e49 leaves the input reserve at 1e49, and the contract has no owner and no reset. Measured, the next ordinary swap instead resets that side to the floor and pays out in full. The gas limit is 85000, read from eth_estimateGas rather than from a receipt. GasUsed is the post-refund charge and a transaction carries the pre-refund peak; sizing from a receipt put an earlier draft 20% under what its own swap needed. An account's first swap needs 79988 and every later one needs 45177, so a run in steady state declares about 44% more gas than it spends. PLT-1093 carries the prewarm change that would close that. PLT-1092 carries the chain-parameter exposure, which is the whole package rather than this constant. Every guard here was broken on purpose before it was believed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR SummaryLow Risk Overview The contract targets UniswapV2-like gas/storage (reserves, per-caller balances, Fixes ERC721 mint gas from 22,460 to 75,000 (measured cold mint ~69k), with a test so under-limited mints can’t silently “succeed” when receipt tracking is off. Reviewed by Cursor Bugbot for commit 6c7e41d. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Adds a well-documented constant-product AMM scenario (contract, binding, scenario, operation set, tests) and raises the ERC721 mint gas limit from a value that made every mint fail. The code is correct and well tested for chains running default EVM storage costs; the main open items are the hard-coded gas limits on Sei's public chains and missing user-facing docs for the new scenario.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The new
ammscenario and itsoperationskeys (swap_a_to_b/swap_b_to_a) are not documented for operators: README's "Available Scenarios" list is not updated, andconfig/doc.go's "Operation baskets" section still names onlystoragerwas declaring a frozen operation vocabulary. Since the operation names and their declaration order are a frozen wire contract,amm's set is worth writing down alongsidestoragerw's. - [suggestion] The scenario deploys exactly one pair, so every AMM transaction in a run writes the same two reserve slots (
reserveA/reserveB). Under Sei's parallel execution that makes the whole AMM slice fully conflicting and serialized, which is faithful to a single UniswapV2 pair but not to a DeFi load profile spread across many pairs. Worth either documenting explicitly next toAMMScenarioor leaving a note for a future multi-pair axis, so a profile author does not read AMM throughput as parallelizable DeFi load. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
- 2 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
generator/scenarios/ERC20Conflict.goandgenerator/scenarios/ERC20Noop.gocarry the same under-sized hard-coded gas limits this PR fixes for ERC721 (the PR body measures ERC20Noop short by 455 gas and ERC20Conflict by 69,710). WithtrackReceiptsdefaulting to false, runs using those scenarios report failed, limit-burning transactions as sent. Tracked as PLT-1091. - [suggestion] README's "Available Scenarios" list (README.md:145-151) was already stale before this PR — it omits
StorageRW,Disperse,EVMTransferFastandEVMTransferNoop, so it is not a reliable index of what the factory registers.
| // | ||
| // Estimating per transaction would put an eth_estimateGas on the send path, | ||
| // which is the load this tool exists to avoid adding. | ||
| const ammSwapGas = 85_000 |
There was a problem hiding this comment.
[suggestion] This limit is calibrated for the default 20,000 SSTORE_SET, and the comment above it states that pacific-1 and atlantic-2 charge ~74,700, putting an account's first swap near 185,000. On those chains every account's first swap will land with a failed status and burn the full 85,000, and with trackReceipts defaulting to false the run reports each one as sent — the exact silent failure mode the ERC721 change in this PR fixes.
I understand the exposure is package-wide and PLT-1092 covers it, so this need not block. But a brand-new scenario shipping a limit already known to fail on the two main public chains is worth at least a guard rather than only a comment: e.g. a one-time eth_estimateGas at deploy/bind time to size the constant (off the per-tx send path), or a startup warning when the scenario runs against a chain whose SSTORE_SET is above the default.
| // This constant read 22460 until it was measured. At that value every mint | ||
| // landed in a block with a failed status and burned the whole limit, and a run | ||
| // with trackReceipts off reported each one as a success. | ||
| const erc721MintGas = 75_000 |
There was a problem hiding this comment.
[suggestion] Good fix — 22,460 was clearly wrong. Note the fix is only complete for chains running default storage costs: a mint writes _owners[tokenId] (always zero→non-zero) and _balances[to] (zero→non-zero for a fresh receiver), so on pacific-1/atlantic-2 at ~74,700 per zero→non-zero write a cold mint needs roughly 180k and still fails silently under 75,000. Worth stating that residual gap in this comment the way ammSwapGas does, so the next reader does not take the constant as chain-independent.
|
Both findings are correct, and both are fixed in #73 rather than here — flagging the trail, since a reviewer reading this PR alone sees suggestions against constants that still exist in this diff. On One correction to the framing: the exposure is not limited to pacific-1 and atlantic-2. I measured arctic-1 by linear fit over On Your ask was to state the residual in the comment. I'd argue deleting the constant beats documenting it, which is what #73 does. What the comment taught — that an estimate is a required limit rather than a receipt's At the top of the stack neither constant exists: Pulling that work down into this PR would defeat the point of stacking it, so I'm leaving this one as the scenario plus the constant fix it was. |
#71 merged squashed, so its content reached main under a new commit and this branch's history no longer shares it. Without this merge the pull request diffs the AMM scenario again alongside the health endpoints.
Stacked on #71. Base is `brandon2/amm-swap-scenario`, so this branch carries the AMM scenario, the ERC721 gas fix and these endpoints together, and the image it builds is the one to deploy. ## Why `sei-load` serves `/metrics` and nothing else. A deployment therefore has no way to tell a run that is still starting from one that is stuck, and a pod counts as available the moment its container starts. That blocks the load-generator deployment in the platform repo, whose pod spec has a `startupProbe`, a `readinessProbe` and a `livenessProbe` pointing at `/readyz` and `/healthz`. Against the current binary every one of them fails. ## What the two endpoints mean `/healthz` answers as soon as the HTTP server binds. It never reads the startup sequence. `/readyz` refuses until the dispatcher is running, and while it refuses it names the phase. Keeping them separate is the point rather than a detail. Funding, deployment and prewarm take minutes against a cold chain. A liveness probe that reported the run dead for that window would restart the pod before it sent a transaction, then restart the next attempt at the same place. The run would never happen, and the cause would read as a crash loop rather than a slow start. Measured against the binary, polling both endpoints across a real startup: ``` t+3s healthz=200 readyz=prewarming accounts [503] t+6s healthz=200 readyz=prewarming accounts [503] ... t+18s healthz=200 readyz=prewarming accounts [503] t+21s healthz=200 readyz=running [200] SIGTERM -> exit 0 ``` The phase in the body is there for the operator watching that window. A ten-minute startup that answers only `503` says nothing about which step is slow. ## One design note The ready flag and the phase are stored as a single value, not as two atomics. Two would leave a window where a writer has set the flag but not yet the phase, so a reader sees the run serving while the body still names the step it left. The status line and the body would then disagree about the same instant. ## Phases | phase | reported while | |---|---| | `starting` | before the run reaches its first step | | `deploying contracts` | the generator deploys what the profile names | | `funding accounts` | the funder pays the account pool | | `prewarming accounts` | prewarm sends one transaction per account | | `running` | ready | | `shutting down` | after SIGTERM, through the post-summary scrape hold | `shutting down` drops readiness while `/healthz` keeps answering. The run holds the pod open on purpose for that scrape window, and a liveness probe that failed during it would kill the process before its final metrics were read. ## Verification Five mutations, five caught: * liveness made to wait for startup * readiness made to always pass * readiness made to stop naming the phase * `NotReady` made a no-op * the single stored value split into two atomics — a reader observed a serving status carrying `funding accounts` The fifth is worth naming. The first version of that guard passed against the split-atomics mutation, so its failure message claimed something it could not detect. It was rewritten to widen the window before it was believed. `gofmt`, `go vet` and `golangci-lint run` are clean. The full suite passes, and the health package passes under `-race`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
#71 and #72 both merged squashed, so their content reached main under new commits and this branch's history no longer shares it. Git therefore saw the scenario files as added on both sides. Resolved toward this branch for AMM.go, AMM_test.go and ERC721.go, which carry the same contracts with their hard-coded gas constants replaced by the measured path — main holds the earlier form. Removed ERC721_test.go, which pinned a constant this branch deletes and could not compile against it. Took main's deferred NotReady in main.go. That fix landed in #72 after this branch was cut, and it covers every exit rather than the signal path alone.
Adds an
ammscenario so a DeFi load profile has a contract to drive, and fixesan existing gas limit that made every ERC721 mint fail.
The AMM contract
A constant-product pair with the storage and gas shape of a UniswapV2 swap: both
reserves, the caller's balance in each token, and an event. It reproduces the
cost, not the economics. There is no router, no fee split, no price oracle and no
minimum-output check.
Two design points are worth stating, because both were wrong in an earlier draft.
The balances wrap rather than check. Nothing reads them back, and a load
generator that fails on its own accounting stops measuring the chain. The draft
credited a short caller exactly what it then debited, which returns the slot to
the value it held. For a caller starting at zero, that is zero. A zero to
non-zero storage write costs four times one that changes a slot already holding a
value, and the default operation mix draws one direction, so that expensive write
would have landed on every swap of the run rather than on the first.
The reserves sit between a floor and a ceiling. The draft had only a floor.
Measured over a long run, the input side then grows without bound and the output
halves every 100,000 swaps, so the profile stops pricing anything like it did at
the start. The ceiling also keeps one oversized call from ending the pair: a swap
of 1e49 leaves the input reserve at 1e49, and the contract has no owner and no
reset. Measured, the next ordinary swap now resets that side to the floor and
pays out in full.
Gas
The limit is a required limit read from
eth_estimateGas, not a receipt'sGasUsed.GasUsedis what the chain charges after the refund lands at the endof execution, and a transaction has to carry the peak before it. Sizing from a
receipt put an earlier draft 20% under what its own swap needed, which fails the
transaction and burns the whole limit.
Measured over six accounts against the deployed binding, on a chain running the
default storage gas costs:
One limit has to cover the higher shape, so a run in steady state declares about
44% more gas than it spends. Priming both slots during prewarm would close that;
it needs a transaction class the prewarm path does not have yet, and PLT-1093
carries it.
The calibration assumes the default 20,000 for a zero to non-zero storage write.
Sei sets that as a chain parameter and the public chains charge about 74,700, so
these numbers are right for arctic-1 and wrong for pacific-1. That exposure is
the whole package rather than this constant, and PLT-1092 covers it.
The ERC721 fix
ERC721.godeclared 22,460 gas for a mint, which isERC20Noop's constantcopied. A mint needs 69,319 to a receiver holding none of the token. Every mint
the scenario sent landed in a block with a failed status having burned the whole
limit, and
trackReceiptsdefaults to false, so the run reported each one assent.
It is fixed here rather than deferred because the
tokenopsprofile this workdeploys uses that scenario, and shipping it broken would read as a chain
regression on the canary.
ERC20Noopis short by 455 gas andERC20Conflictby 69,710. Neither appears ina profile deployed today, so PLT-1091 carries them.
What is checked, and what is not
Every guard was broken on purpose before it was believed. Six mutations, six
caught: the ERC721 limit back to 22,460 and up to 200,000; the AMM limit down to
the steady shape and up past the band; the B-to-A leg made unreachable; the
operation stamp dropped.
The reserve bounds and the balance-slot property have no guard. They were
measured by hand and no test repeats the measurement, because the repo runs no
contract against an EVM in CI. A Go test on
ethclient/simulatedguards allthree and is written, but it adds about 35 indirect modules including the full
go-ethereum node stack, which is too large a dependency change to make in
passing. PLT-1094 carries the harness question and the parked test.
Verification
gofmt,go vet,staticcheckandgolangci-lint runare clean. The fullsuite passes. All seven tracked bindings regenerate byte-identical under the
pinned solc 0.8.19 and abigen 1.16.1, so
check-bindingspasses.🤖 Generated with Claude Code