Skip to content

fix: normalize protocol addresses in cancelOrders batches - #1999

Closed
Mabolla wants to merge 2 commits into
ProjectOpenSea:mainfrom
Mabolla:agent/fix-cancel-protocol-address-casing
Closed

fix: normalize protocol addresses in cancelOrders batches#1999
Mabolla wants to merge 2 commits into
ProjectOpenSea:mainfrom
Mabolla:agent/fix-cancel-protocol-address-casing

Conversation

@Mabolla

@Mabolla Mabolla commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Thanks for opening a PR!

We really appreciate you taking the time to contribute. It means a lot to the OpenSea team and the broader developer community.

A quick note about how this repo works

This repository is a read-only mirror of a package maintained in an internal monorepo. Because of that, pull requests cannot be merged directly here.

But don't worry -- your contribution won't be lost! Here's what happens next:

  1. Our team reviews every PR that comes in.
  2. If the change looks good, we'll recreate it internally in our monorepo.
  3. The fix will be synced back to this public repo on the next release.

We'll keep you posted on the PR as things progress.

Is this a bug report?

If you're reporting a bug rather than submitting a code fix, opening an issue is usually the fastest path to a resolution. Bug report issues help us triage and prioritize effectively.

Thanks again for helping make OpenSea better for everyone!

Summary

Fixes cancelOrders() rejecting a batch when its orders use differently-cased forms of the same Seaport protocol address.

Root cause

cancelOrders() validates each protocol address with requireValidProtocol(), which treats equivalent Ethereum address casing correctly, but then stores the original strings directly in a Set.

As a result, the same protocol address in lowercase/checksummed and uppercase form produced two entries, causing the batch to fail with:

All orders in a cancelOrders batch must share the same protocolAddress.

even though both orders refer to the same contract.

Changes

  • Normalize validated protocol addresses with checksumAddress() before adding them to the batch protocol set.
  • Preserve the existing rejection for genuinely different protocol addresses.
  • Add a regression test using differently-cased forms of the same Seaport address.

Validation

  • Regression test fails before the fix because the two casing variants are treated as different protocols.
  • Targeted regression test: 1/1 passed after the fix.
  • Full unit suite: 946/946 passed across 42 test files.
  • Targeted Biome check passes for the changed files.

@ryanio

ryanio commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

Thanks. This one went in essentially as you wrote it, recreated in our internal monorepo as PR 649 (private repo, so that number will not resolve from here) and merged.

The detail you caught is that requireValidProtocol() compares addresses correctly, so validation passed, and then the raw string went into the Set. One protocol counted as several depending on how each order happened to spell it, and a batch that was entirely valid got refused with a message claiming its orders did not share a protocol address. The validation and the bookkeeping disagreed about what an address is, and only the bookkeeping was wrong.

checksumAddress() at the add call is the right place to normalize, since it leaves requireValidProtocol owning validation and leaves genuinely different protocols rejected. The regression test is well chosen too: asserting that the run reaches requireAccountIsAvailable exactly once proves the batch stayed one batch, rather than only proving the protocol error did not fire.

This repo is a mirror that publishes on release, so the change appears here on the next one.

@ryanio ryanio closed this Aug 23, 2026
pull Bot pushed a commit to dmrazzy/opensea-js that referenced this pull request Aug 23, 2026
Origin-SHA: ff532b127211ea20b8e32969990819beb8bb15d5

### Patch Changes

- bbbbfea: Pin the wire shape of the agent handshake bodies.

  `proposeAgentRelationship` and `confirmAgentRelationship` take camelCase, like every other default-path write, and `Fetcher.request` converts to the snake_case the API requires. That was already the behavior, but nothing tested it end to end: the existing specs stub the fetcher, so they show the body being handed straight to `request` and never show the conversion. Reading the method alone, a caller can reasonably conclude they must supply `{counterparty_address, caller_role}` themselves, and the API answers a camelCase body with 400 "Missing required field 'counterparty_address'", which does not name the real problem.

  The new test drives the real fetcher and asserts the exact bytes. No behavior change.

- 34d3a50: `cancelOrders` no longer rejects a batch whose orders name the same Seaport protocol address in different letter cases. The addresses were grouped in a `Set` keyed on the raw string, so one protocol counted as several. Thanks to @Mabolla (ProjectOpenSea#1999).
- 34d3a50: Providers: infer the EIP-712 primary type as the root of the type graph instead of taking the first key in `types`. Both the viem adapter and the seaport bridge signed the wrong struct when a dependency was declared before the root, and neither refused an ambiguous or circular type set. Both now share one helper and throw rather than guess, matching what ethers does with the same input. Thanks to @Mabolla (ProjectOpenSea#1998, ProjectOpenSea#2000).
- 3cc8640: The wire-shape guard added in ProjectOpenSea#646 observes which `input_data` keys the ERC20
  preflight reads by handing it a recording Proxy. That Proxy only had a `get`
  trap, so a refactor to `Object.keys(inputData)` or an `in` test would have
  reached a key without the guard seeing it. It now traps `ownKeys` and `has` too.

  Tests only, no behaviour change.

- ea967e2: The ERC20 fulfillment preflight is now tested against captured
  `POST /api/v2/listings/fulfillment_data` responses rather than a hand-written
  guess at their shape. Four response bodies are committed verbatim, covering both
  call shapes a single-listing fulfillment can return and both an ERC20-priced and
  a native-priced form of each, and the expected payment total comes from the
  listing price a separate endpoint reports.

  A new `erc20FulfillmentWireShape` suite checks every top-level `input_data` key
  the preflight reads against the `input_data` variants declared in the OpenAPI
  spec, so a name the API does not send has to be justified as a deliberate alias
  instead of silently disabling the guard, which is what shipped in ProjectOpenSea#638.

  No behaviour change and no change to the public API. The keys the check inspects
  are observed at runtime through a recording Proxy rather than read out of the
  source text, so a rename or an extracted helper cannot quietly narrow what gets
  checked.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: devorun <130918800+devorun@users.noreply.github.com>
Co-authored-by: Mabolla <Mabolla@users.noreply.github.com>
Co-authored-by: MrFaruk0 <MrFaruk0@users.noreply.github.com>
Co-authored-by: omerbek <omerbek@users.noreply.github.com>
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.

2 participants