Skip to content

docs: overhaul the README, summarise the divergence, prune .env.example - #54

Merged
mfw78 merged 3 commits into
developfrom
docs/upstream-divergence
Aug 1, 2026
Merged

docs: overhaul the README, summarise the divergence, prune .env.example#54
mfw78 merged 3 commits into
developfrom
docs/upstream-divergence

Conversation

@mfw78

@mfw78 mfw78 commented Aug 1, 2026

Copy link
Copy Markdown

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:

  • The TWAP Data struct was missing bytes32 appData. The README told you to ABI-encode that struct as staticInput. Encoding the nine documented fields instead of ten produces the wrong bytes, so the documented recipe could not have worked.
  • getTradeableOrderWithSignature was 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 a PollResult and reverts only for authorisation and handler-interface failures. Anyone following the README would have wrapped a try/catch around a call that no longer throws, and silently never handled the non-POST verdicts.
  • Proof was documented as location 0 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.
  • There are no fork tests. Nothing in test/ calls createSelectFork or reads ETH_RPC_URL, and an unfiltered run gives the same 166 tests as one excluding fork. 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.
  • "Deterministic deployment" described replaying a mainnet creation transaction to reproduce upstream's official addresses, and cited "issue feat!: URI-based proof payload locations with blob publication verification聽#39" while linking issue 93. This fork has no official addresses to reproduce, so the section is removed rather than repaired. The related "won't deploy to the official addresses" warning goes for the same reason.
  • broadcast/deploy_OrderTypes.s.sol/ does not exist. The section 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.
  • #Conditional-order-cancellation did not resolve, since GitHub slugs are lower-case.
  • Environment setup pointed at .env.example wholesale. It now names the variables the scripts actually read: PRIVATE_KEY, SETTLEMENT, SAFE, TWAP, COMPOSABLE_COW, ETHERSCAN_API_KEY.
  • Architecture linked only to a HackMD page predating this work, and now points at docs/architecture.md, docs/discovery.md and docs/design/.
  • deploy_ExtensibleFallbackHandler and deploy_ValueFactories exist but were undocumented.
  • ```solidity= is a HackMD directive GitHub does not recognise as a language tag.

.env.example

Third commit. Roughly half the file configured upstream's watch-tower rather than anything here: 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, a Sentry DSN. Eleven variables, none read by any script, test or workflow in this repository, so copying it to .env mostly 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_URL also no longer defaults to http://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

  • Every relative link target resolves; both anchors match a real heading.
  • Both documented test commands were run: 123 and 166.
  • generateOrder is declared inside interface IConditionalOrderGenerator, and no getTradeableOrder remains in src/.
  • Zero-amount guards resolve to OrderNotValid(ZeroAmount.selector) in GoodAfterTime, StopLoss, TradeAboveThreshold and TWAP.
  • "Shape-compatible" settlement path rests on isValidSafeSignature being signature-identical to upstream/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 --check clean.

Base automatically changed from docs/watch-tower-rename to develop August 1, 2026 01:08
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
mfw78 force-pushed the docs/upstream-divergence branch from 6f92c1b to 480c289 Compare August 1, 2026 01:10
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.
@mfw78 mfw78 changed the title docs: summarise the divergence from upstream in the README docs: overhaul the README and summarise the divergence from upstream Aug 1, 2026
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.
@mfw78 mfw78 changed the title docs: overhaul the README and summarise the divergence from upstream docs: overhaul the README, summarise the divergence, prune .env.example Aug 1, 2026
@mfw78
mfw78 merged commit fa3dbb9 into develop Aug 1, 2026
1 check passed
@mfw78
mfw78 deleted the docs/upstream-divergence branch August 1, 2026 02:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant