docs: overhaul the README, summarise the divergence, prune .env.example - #54
Merged
Conversation
The warning added in #46 says the fork has diverged substantially and lists the surfaces in one sentence, which is enough to stop someone deploying it but not enough to tell them what is actually different. A reader arriving from cowprotocol/composable-cow had to reconstruct that from the commit log. Adds a "Divergence from upstream" section, one line per change, covering order generation, typed errors, structured polling, the fill overlay, the order manifest, handler discovery, proof payload locations, handler validation and housekeeping. The warning now points at it. Also records that upstream has since added ComposableCowPoller and that this tree does not have it. The divergence runs both ways and a section that only lists what was added here would read as a superset. Two parts of the README described interfaces that no longer exist, which is what prompted checking the rest of it: - The merkle root instructions still described `Proof` as a `location` of 0 or 1 with an opaque payload. It has been `{uris, blobVersionedHashes}` since #39. - `getTradeableOrderWithSignature` was described as reverting with custom errors for order conditions. Since #23 it returns a structured `PollResult` and reverts only for authorisation and handler-interface failures, so the documented integration was the one the change replaced. Every claim in the new section was checked against the tree rather than the commit subjects: the settlement entry point is signature-identical to upstream, which is what "shape-compatible" rests on.
mfw78
force-pushed
the
docs/upstream-divergence
branch
from
August 1, 2026 01:10
6f92c1b to
480c289
Compare
Removes hard line wrapping from the divergence section added in the previous commit. The rest of the file has never been wrapped, and mixing the two makes paragraphs reflow inconsistently once rendered. Corrections, each checked against the tree rather than assumed: - The TWAP `Data` struct was missing `bytes32 appData`, which the struct has carried since it was added. Anyone ABI-encoding `staticInput` from the README would have produced the wrong bytes, so this is the most damaging of these. - There are no fork tests. Nothing in `test/` calls `createSelectFork` or reads `ETH_RPC_URL`, and an unfiltered run yields the same 166 tests as one excluding `fork`. The three-command ladder therefore documented a distinction that does not exist, and the archive-node requirement was inherited from upstream. Now two commands, verified at 123 and 166. - `Data` was fenced as ```solidity=`, a HackMD directive that GitHub does not recognise as a language tag. - "Deterministic deployment" described reproducing upstream's official addresses by replaying a mainnet creation transaction, and cited "issue #39" while linking issue 93. This fork has no official addresses to reproduce, so the section is removed rather than repaired. - "won't deploy the contracts to the official addresses" dropped from script-based deployment for the same reason. - Runs were said to land in `broadcast/deploy_OrderTypes.s.sol/`, which does not exist, and to be "updated manually". Now describes what `broadcast/` actually holds, which is upstream's records. - A dangling `**TODO**` and a link to a `[CLI](#CLI)` section that has never existed in this file. - The cancellation cross-reference used `#Conditional-order-cancellation`. GitHub slugs are lower-case, so the link did not resolve. - Environment setup pointed at `.env.example` wholesale. It now names the variables the scripts actually read: `PRIVATE_KEY`, `SETTLEMENT`, `SAFE`, `TWAP`, `COMPOSABLE_COW` and `ETHERSCAN_API_KEY`. - Architecture linked only to a HackMD page predating this work. It now points at `docs/architecture.md`, `docs/discovery.md` and `docs/design/`. - `deploy_ExtensibleFallbackHandler` and `deploy_ValueFactories` exist but were undocumented. `.env.example` is left alone deliberately: roughly half of it is Tenderly, Slack, Sentry and Goerli configuration belonging to upstream's watch-tower, and none of it is read by anything in this repository. Cleaning it is a separate change.
Roughly half the file configured upstream's off-chain watch-tower rather than anything in this repository: Tenderly node URLs and credentials, a `NETWORK` selector defaulting to Goerli, `BLOCK_NUMBER` and `STORAGE` for replaying a block in WATCH mode, a Slack webhook and a Sentry DSN. None of the eleven variables is read by any script, test or workflow here, so copying the file to `.env` produced mostly settings with nothing to act on them. What remains is what the tooling actually reads, each annotated with its consumer: `PRIVATE_KEY` for every script in `script/`, `SETTLEMENT` for `ComposableCow`'s constructor, `COMPOSABLE_COW` for deploying order types against it, `SAFE` and `TWAP` for single order submission, `ETHERSCAN_API_KEY` for verification, and `ETH_RPC_URL` for the `--rpc-url` argument. `ETH_RPC_URL` no longer defaults to `http://erigon.dappnode:8545`, a host specific to whoever wrote the file, and one that fails in a way that looks like a broken script rather than an unset variable. Also corrects the README sentence added in the previous commit, which attributed `COMPOSABLE_COW` to single order submission alone when `deploy_OrderTypes` reads it too.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two commits. The first adds a divergence summary; the second unwraps it and overhauls the rest of the file, which turned out to be carrying a fair amount of stale and inherited content.
Divergence section
One line per change: order generation, typed errors, structured polling, the fill overlay, the order manifest, handler discovery, proof payload locations, handler validation, and housekeeping. The warning at the top links to it.
It also records that upstream has since added
ComposableCowPoller(polling schedules and just-in-time order funding, upstream PRs 122 to 125) and that this tree does not have it. The divergence runs both ways, and a section listing only what was added here would read as a superset of upstream, which is not true.Corrections
Each was checked against the tree rather than assumed. Ordered by how much damage it could do:
Datastruct was missingbytes32 appData. The README told you to ABI-encode that struct asstaticInput. Encoding the nine documented fields instead of ten produces the wrong bytes, so the documented recipe could not have worked.getTradeableOrderWithSignaturewas documented as reverting with custom errors. Since feat!: add structured poll interface for watch-towers聽#23 it does not revert for order conditions at all; it returns aPollResultand reverts only for authorisation and handler-interface failures. Anyone following the README would have wrapped atry/catcharound a call that no longer throws, and silently never handled the non-POSTverdicts.Proofwas documented aslocation0 or 1 with an opaque payload. Since feat!: URI-based proof payload locations with blob publication verification聽#39 it is{uris, blobVersionedHashes}, both channels optional, blob hashes verified attached to the setting transaction.test/callscreateSelectForkor readsETH_RPC_URL, and an unfiltered run gives the same 166 tests as one excludingfork. The three-command ladder documented a distinction that does not exist and an archive node requirement inherited from upstream. Now two commands, verified at 123 unit and 166 with fuzz.broadcast/deploy_OrderTypes.s.sol/does not exist. The section now describes whatbroadcast/actually holds, which is upstream's records.**TODO**and a link to a[CLI](#CLI)section that has never existed in this file.#Conditional-order-cancellationdid not resolve, since GitHub slugs are lower-case..env.examplewholesale. It now names the variables the scripts actually read:PRIVATE_KEY,SETTLEMENT,SAFE,TWAP,COMPOSABLE_COW,ETHERSCAN_API_KEY.docs/architecture.md,docs/discovery.mdanddocs/design/.deploy_ExtensibleFallbackHandleranddeploy_ValueFactoriesexist but were undocumented.```solidity=is a HackMD directive GitHub does not recognise as a language tag..env.exampleThird commit. Roughly half the file configured upstream's watch-tower rather than anything here: Tenderly node URLs and credentials, a
NETWORKselector defaulting to Goerli,BLOCK_NUMBERandSTORAGEfor replaying a block in WATCH mode, a Slack webhook, a Sentry DSN. Eleven variables, none read by any script, test or workflow in this repository, so copying it to.envmostly produced settings with nothing to act on them.What remains is annotated with its consumer. Checked both directions: every retained variable has real references, every removed one had zero.
ETH_RPC_URLalso no longer defaults tohttp://erigon.dappnode:8545, a host specific to whoever wrote the file, which fails in a way that looks like a broken script rather than an unset variable.Checking
generateOrderis declared insideinterface IConditionalOrderGenerator, and nogetTradeableOrderremains insrc/.OrderNotValid(ZeroAmount.selector)inGoodAfterTime,StopLoss,TradeAboveThresholdandTWAP.isValidSafeSignaturebeing signature-identical toupstream/main, diffed directly. That is the claim most worth challenging in review, since it is what a reader would lean on when judging how much of an existing integration survives.README only.
forge fmt --checkclean.