refactor: return SourcifyChainMap from initializeSourcifyChains#2749
Merged
kuzdogan merged 8 commits intofeat/remote-chain-config-v2from Apr 16, 2026
Merged
Conversation
Instead of mutating a module-level `sourcifyChainsMap` singleton, `initializeSourcifyChains()` now returns a fresh `SourcifyChainMap`. Key changes: - `sourcify-chains.ts`: remove exported `sourcifyChainsMap` const; return a local map from the function; drop the map-clearing loop; clean up the non-null assertion on `chainsExtensions`; use consistent string keys for all chainId lookups - `cli.ts`: capture the returned map in the async IIFE; make `getEtherscanApiKeyForEachChain` accept the map as a parameter - `ServerFixture`: store the returned map on `this.sourcifyChainsMap`; integration tests access it via the fixture instead of a module import - `chain-tests.spec.ts` / `etherscan-instances.spec.ts`: use the returned value directly; add `.catch()` to async IIFEs so failures exit instead of hanging Mocha with --delay - Unit tests: use returned value for assertions; remove the beforeEach map-clearing loop; rewrite the re-initialization test as two independent-map assertion Addresses Manuel's review comment on #2709. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… chain map The async IIFE + run() pattern relied on Mocha's --delay flag which was never set in the test scripts, making test registration order a race condition. Replace it with a plain synchronous describe loop that uses only chainId at registration time. Chain names are recovered inside before() once serverFixture has initialized its sourcifyChainsMap, then written back onto the suite title so reporters (spec, mochawesome) still show the human-readable name. The supported-chain guard also moves into before() for the same reason. The "Double check" section now builds sourcifyChainsArray inside the it() block from serverFixture.sourcifyChainsMap, eliminating the last describe-time dependency on the chain map. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… beforeEach it() titles are registered synchronously like describe() titles, so the chain name cannot be embedded at registration time. Store chainName in a closure variable (set by before()), then patch this.currentTest.title in beforeEach() by replacing the chainId placeholder with the full name. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e bug `chainId` is declared with `let` outside the for..in loop, so all describe/before/beforeEach/it closures that run asynchronously share the same variable and see the last iteration's value. Only the final chain (11155420) got the correct title replacement; all others saw the wrong chainId in the replace() call. Fix: capture the current value in `const id = chainId` at the top of each describe callback. All closures inside that describe now close over their own immutable `id`. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- ServerFixture: comment that sourcifyChainsMap is assigned in before() and safe to read inside it/beforeEach - sourcify-chains.spec.ts: assert the two returned maps are distinct object references, not just disjoint in keys Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| maxFileSize: config.get<number>("server.maxFileSize"), | ||
| corsAllowedOrigins: config.get<string[]>("corsAllowedOrigins"), | ||
| chains: fixtureOptions_?.chains || sourcifyChainsMap, | ||
| chains: fixtureOptions_?.chains || this.sourcifyChainsMap, |
Member
There was a problem hiding this comment.
not a problem of this PR, but a problem in general:
below we always pass the this.sourcifyChainsMap to the verification service, but it should also prioritize the fixtureOptions.chains.
I think in general it would be better to remove the sourcifyChainMap from the VerificationServiceOptions and instead pass a separate argument from the Server constructor to the VerificationService in order to avoid such mistakes.
Member
Author
There was a problem hiding this comment.
Do you think it should be in this PR? The change should be straightforward?
Member
Author
There was a problem hiding this comment.
See c16794e about prioritizing fixtureOptions.chains
Above comment for removing the sourcifyChainMap from VerificationServiceOptions
Member
There was a problem hiding this comment.
doesn't have to be in this PR, but should be straight forward, yes. I would simply add the chain map as a separate argument to the VerificationService constructor.
…+ fix fixtureOptions priority - Replace the `sourcifyChainsMap!` instance field with a getter that delegates to `this.server.chainRepository.sourcifyChainMap` — the field was redundant since the server already owns the chain map. - Create a local `sourcifyChainsMap` const in before() that respects `fixtureOptions_?.chains` first, then falls back to `initializeSourcifyChains()`. Both `serverOptions.chains` and `sourcifyChainMap` in VerificationServiceOptions now use the same value, fixing the inconsistency where the override was only applied to one of them. - Replace all `serverFixture.server.chainRepository.sourcifyChainMap` call sites across integration tests with `serverFixture.sourcifyChainsMap` for consistency. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
manuelwedler
approved these changes
Apr 16, 2026
github-project-automation
bot
moved this from Triage
to Sprint - Done
in Sourcify Public
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
2 participants
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.
Summary
Addresses Manuel's review comment on #2709 (
cli.ts:99):And the follow-up on
sourcify-chains.ts:277:What changed
src/sourcify-chains.ts: Remove the exportedsourcifyChainsMapsingleton.initializeSourcifyChains()now returnsPromise<SourcifyChainMap>. The map-clearing loop is gone (each call builds a fresh local map). The unusualif (!chainsExtensions!)non-null assertion is cleaned up tolet chainsExtensions: ... | undefined+if (!chainsExtensions). All chainId keys are now consistently strings (chainId.toString()).src/server/cli.ts: Capture the returned map withconst sourcifyChainsMap = await initializeSourcifyChains().getEtherscanApiKeyForEachChainnow accepts the map as a parameter rather than closing over the module-level export.test/helpers/ServerFixture.ts: Store the returned map onthis.sourcifyChainsMap(public field). Integration tests access it viaserverFixture.sourcifyChainsMap[...].sourcify-chains.spec.ts,verificationWorker.spec.ts,contract-creation-util.spec.ts): Use the returned value; remove thebeforeEachmap-clearing loop; rewrite the re-initialization test as two independent-map assertions.chain-tests.spec.ts,etherscan-instances.spec.ts): Use the returned value in the async IIFEs. Add.catch()to both IIFEs so a chain-init failure exits instead of hanging Mocha's--delaymode.etherscan-instances.spec.tsis now wrapped in the same--delay/run()pattern aschain-tests.spec.ts(was previously relying on synchronous access to the old module-level map).test/chains/deployContracts.ts: CallinitializeSourcifyChains()insidemain()and use the returned map.Out of scope
Other review comments from #2709 (GCP-hosted URL, not importing
configinsourcify-chains.ts, Dockerfile dedup, etc.) will be addressed separately.🤖 Generated with Claude Code