Skip to content

Fix SignerChanged event, refresh tests for OZ v5, add BSC deploy script - #8

Open
AmazingAng wants to merge 4 commits into
mainfrom
upgrade/2026-07
Open

Fix SignerChanged event, refresh tests for OZ v5, add BSC deploy script#8
AmazingAng wants to merge 4 commits into
mainfrom
upgrade/2026-07

Conversation

@AmazingAng

Copy link
Copy Markdown
Member

Prepares the SBT contracts for the BNB Chain deployment and fixes one real bug plus a stale test suite.

Contract fix

WTFSBT1155Minter.setSigner emitted SignerChanged(signer, newSigner) after overwriting signer, so every event reported the new signer as both the old and new value — anyone indexing signer rotations got useless data. It now captures the old value first.

No other contract changes: the contracts are already chain-agnostic (OpenZeppelin v5, block.chainid cached at construction), so moving to BNB Chain needs no logic changes.

Tests: 13 failing → 20/20 passing

The suite had drifted from OpenZeppelin v5 and two tests were not testing what they claimed:

  • Seven tests asserted v4 revert strings ("Ownable: caller is not the owner", "Pausable: paused", "ERC1155: caller is not owner nor approved") against contracts that now emit v5 custom errors — updated to abi.encodeWithSelector.
  • testUnauthorizedMinter signed with the configured signer's private key, so it never exercised the unauthorized path — now signs with a non-signer key.
  • testRecoverWithNoTokens expected a "No tokens to recover" revert that the contract does not implement; recovering an empty address is a harmless no-op, so it is asserted as such.
  • testExpiredSignature set a non-zero mint price and reverted on "Donation too low" before ever reaching the deadline check — price set to zero so the deadline is what fails.
  • testNonceReusage expected "Invalid nonce"; the contract consumes nonces via _useNonce, so a stale nonce produces a signer mismatch and reverts with "Invalid signature". Replay is still prevented — only the revert reason differs.

Deployment

script/Deploy.s.sol deploys WTFSBT1155 and WTFSBT1155Minter and grants the minter role in one run, driven by TREASURY, SIGNER and optional SBT_NAME / SBT_SYMBOL / SBT_BASE_URI environment variables. SIGNER must match the backend signing key. foundry.toml gains bsc and bsc_testnet RPC endpoints:

TREASURY=0x... SIGNER=0x... forge script script/Deploy.s.sol \
  --rpc-url bsc_testnet --broadcast --private-key $DEPLOYER_KEY

Verified by a local dry run. After deploying, register each certificate with createSoul and keep the soulId to course mapping in sync with the backend SBT.SoulCourse config.

One thing to decide before mainnet

WTFSBT1155 pays the treasury with transfer, which forwards only 2300 gas. If the treasury is (or later becomes) a multisig such as a Gnosis Safe, donations will revert. Worth switching to a call-based transfer before deploying if that is the plan.

🤖 Generated with Claude Code

- setSigner emitted (newSigner, newSigner); it now captures the old
  signer before overwriting it
- Tests updated to OpenZeppelin v5 custom errors; testUnauthorizedMinter
  previously signed with the configured signer's key and never exercised
  the unauthorized path; recover-with-no-tokens is asserted as a no-op
  (20/20 passing)
- script/Deploy.s.sol: env-driven deploy (TREASURY, SIGNER,
  SBT_BASE_URI) that wires WTFSBT1155 + Minter and grants the minter role
- foundry.toml: bsc and bsc_testnet RPC endpoints

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Legacy certificate holders on Base need to be re-issued on BNB Chain,
which needs batch minting plus a trustworthy holder export.

Contract:
- WTFSBT1155.batchMint(address[] to, uint256[] soulIds), minter-only,
  non-payable: mints one certificate per pair and skips recipients who
  already hold the soul, so a partially completed airdrop can be re-run
  safely. Emits BatchMinted(count)
- The created/mint-window checks shared with mint() are extracted into
  _requireMintable so the two can never diverge; mint()'s signature and
  revert strings are unchanged, keeping the frontend error mapping valid

Tooling:
- script/export-base-holders.sh reconstructs holders from Base
  TransferSingle/TransferBatch logs using cast, chunking eth_getLogs
  (public RPC caps ranges at 10k blocks), caching chunks so interrupted
  runs resume, and halving a chunk on a range error. --dry-run prints a
  per-soulId summary
- script/Airdrop.s.sol mints the exported set in CHUNK_SIZE batches
  (default 100), refuses to run unless the broadcaster is a minter or
  while the contract is paused, validates soulIds against
  latestUnusedTokenId, and supports an optional SOUL_ID_MAP
- airdrop/README.md documents the runbook and the soulId ordering hazard

The legacy SBT is 0xB05D424943350aDfadeC4731CD54f12cC45E8c5c; the address
previously treated as the SBT (0x2BBE57dA...7385d2) is the minter, so
state calls against it revert. The committed export holds 204
certificates (139 for soul 0, 65 for soul 1), matching totalSupply on
Base exactly, verified end to end on anvil: 204 minted, supplies match,
and a re-run mints nothing. Cost is roughly 34k gas per certificate.

Tests: 42 passing (20 existing, 12 batchMint, 10 airdrop script).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AmazingAng

Copy link
Copy Markdown
Member Author

Update: batch minting + Base→BSC airdrop toolchain (413d6a7)

Founder decisions are settled: treasury is a plain EOA (so the contract's transfer payout stays as-is), minting is free with 0.01 BNB suggested as a donation, and legacy Base holders get re-issued on BSC.

WTFSBT1155.batchMint(address[] to, uint256[] soulIds) — minter-only, non-payable, skips recipients who already hold the soul so a partially completed airdrop can be re-run safely. The created/mint-window checks shared with mint() are extracted into _requireMintable, and mint()'s signature and revert strings are unchanged so the frontend's error-string mapping keeps working.

Tooling: script/export-base-holders.sh (cast-only; chunks eth_getLogs around the public Base RPC's 10k-block cap, caches chunks so interrupted runs resume) and script/Airdrop.s.sol (chunked batchMint, refuses to run unless the broadcaster is a minter or while paused, optional SOUL_ID_MAP). Runbook in airdrop/README.md.

Correction worth noting

0x2BBE57dA6DFE615B9cE86B2BD149A953af7385d2 — the address the frontend hardcoded as the "SBT contract" — is actually the minter; state calls against it revert. The real legacy ERC1155 is 0xB05D424943350aDfadeC4731CD54f12cC45E8c5c, found via the minter's wtfsbt() getter. Both are now documented in the frontend constants so the two env vars don't get swapped.

Verification

  • 42 tests passing (20 existing, 12 batchMint, 10 airdrop script)
  • Real export: 204 certificates (139 for soul 0, 65 for soul 1), matching totalSupply on Base exactly, no duplicates or zero addresses
  • Full anvil end-to-end against the real 204-entry file: 204 minted, supplies match Base, re-run minted nothing (idempotent)
  • Measured cost: ~34k gas per certificate (~3.39M for a 100-recipient chunk), so ~7M gas for the whole airdrop

Follow-up for a separate PR

The vendored forge-std is outdated: its console.sol uses old log(string,uint) signatures that silently revert against current Foundry (numeric logs printed nothing), and Vm.sol lacks parseJsonKeys. The script works around both; upgrading the vendored copy would be a clean separate change.

…ed copy

forge-std was in an odd state: a 52-file copy of v1.9.7's predecessor
(v1.5.0) sat at the repo root, tracked as ordinary files and referenced by
nothing, while .gitmodules declared a lib/forge-std submodule that had
never been added. Imports actually resolved through the copy nested inside
the openzeppelin-contracts submodule (v1.7.6), which is why nobody noticed.

That stale copy cost real debugging time while writing the airdrop script:
its console.sol predates the current numeric log signatures, so every
console.log of a number silently reverted and printed nothing, and its
Vm.sol lacks parseJsonKeys.

- lib/forge-std added as a submodule pinned to v1.9.7, matching the
  declaration that was already in .gitmodules
- explicit forge-std/ and ds-test/ remappings so resolution no longer
  depends on whichever version openzeppelin-contracts happens to vendor
- root forge-std/ copy deleted (unreferenced)
- removed the two workarounds the airdrop script needed: the hand-declared
  IVmJsonKeys interface is now vm.parseJsonKeys, and console2 is back to
  console (in 1.9.7 console2 is merely an alias)

Verified: numeric logs print again (Deploy.s.sol now reports chainid),
forge build clean, 42 tests passing.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AmazingAng

Copy link
Copy Markdown
Member Author

forge-std upgraded to v1.9.7 (e0cb2a9)

The dependency was in a stranger state than the earlier note suggested:

  • A 52-file copy of v1.5.0 sat at the repo root, tracked as ordinary files and referenced by nothing.
  • .gitmodules declared a lib/forge-std submodule that had never been added — the directory did not exist.
  • Imports actually resolved through forge-std/=lib/openzeppelin-contracts/lib/forge-std/src/, i.e. whatever version OpenZeppelin happens to vendor (v1.7.6). That is why the mismatch went unnoticed.

That stale resolution cost real debugging time while writing the airdrop script: its console.sol predates the current numeric log signatures, so every console.log of a number silently reverted and printed nothing, and its Vm.sol lacks parseJsonKeys.

Changes:

  • lib/forge-std added as a real submodule pinned to v1.9.7, making the existing .gitmodules declaration true
  • Explicit forge-std/ and ds-test/ remappings, so resolution no longer depends on OpenZeppelin's vendored copy
  • Root forge-std/ directory deleted
  • Both workarounds removed: the hand-declared IVmJsonKeys interface is now just vm.parseJsonKeys, and console2 is back to console (in 1.9.7 console2 is only an alias)

Verified: numeric logs print again (Deploy.s.sol now reports chainid: 31337 where it previously printed nothing), forge build clean, 42 tests passing.

Note for reviewers: this adds a submodule, so a fresh clone needs git submodule update --init --recursive (or forge install).

- .github/workflows/ci.yaml: build --sizes, test, and fmt --check on PRs
  and pushes to main/upgrade/**. Checkout uses submodules: recursive,
  which is now required since forge-std became a real submodule
- forge fmt applied tree-wide in this commit so the check can be
  enforcing rather than advisory. Formatting only; 42 tests still pass

Co-Authored-By: Claude Fable 5 <noreply@anthropic.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.

1 participant