Only initialze EDR's ContractDecoder once per NetworkManager instance#8074
Only initialze EDR's ContractDecoder once per NetworkManager instance#8074alcuadrado merged 4 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: d06cecf The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
alcuadrado
force-pushed
the
single-contract-decoder
branch
from
477e1ee to
5373100
Compare
There was a problem hiding this comment.
Pull request overview
Optimizes EDR provider startup by reusing a single ContractDecoder per NetworkManagerImplementation instance, avoiding repeated build-info loading/decoder construction across successive connections.
Changes:
- Cache and reuse an EDR
ContractDecoderinNetworkManagerImplementationwhen creating EDR-simulated providers. - Update
EdrProvider.createconfiguration to accept a prebuiltcontractDecoderinstead of building it internally. - Add
EdrProvider.createContractDecoderhelper for constructing a decoder from build info buffers.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
v-next/hardhat/src/internal/builtin-plugins/network-manager/network-manager.ts |
Lazily initializes and caches a ContractDecoder, then passes it into each EdrProvider.create call. |
v-next/hardhat/src/internal/builtin-plugins/network-manager/edr/edr-provider.ts |
Refactors provider creation to require an externally provided ContractDecoder and adds a helper constructor. |
| buildInfos: await this.#getBuildInfosAndOutputsAsBuffers(), | ||
| ignoreContracts: false, | ||
| }, | ||
| contractDecoder: this.#contractDecoder, |
There was a problem hiding this comment.
this.#contractDecoder is typed as ContractDecoder | undefined, but EdrProvider.create now requires a non-optional contractDecoder. This will fail type-checking at this call site. Consider storing the initialized decoder in a local const (or changing the field to a non-optional type and using definite assignment) so the argument is guaranteed to be ContractDecoder here.
There was a problem hiding this comment.
this type checks already
| if (this.#contractDecoder === undefined) { | ||
| this.#contractDecoder = await EdrProvider.createContractDecoder({ | ||
| buildInfos: await this.#getBuildInfosAndOutputsAsBuffers(), | ||
| ignoreContracts: false, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The lazy initialization of #contractDecoder isn't concurrency-safe: if two connections create an EDR provider at the same time, both can enter this if block and build/load the decoder in parallel (defeating the "once per NetworkManager" goal). Consider caching a single in-flight Promise (e.g., #contractDecoderPromise) or guarding initialization with a simple mutex to ensure only one initialization occurs.
There was a problem hiding this comment.
The AsyncMutex here and the double guard should avoid the race condition. The call to EdrProvider.createContractDecoder happens with the mutex and must be complete before another callback can reach here because of the mutex.
v-next/hardhat/src/internal/builtin-plugins/network-manager/network-manager.ts
Show resolved
Hide resolved
v-next/hardhat/src/internal/builtin-plugins/network-manager/network-manager.ts
Show resolved
Hide resolved
alcuadrado
added
no docs needed
v-next/hardhat/src/internal/builtin-plugins/network-manager/network-manager.ts
Show resolved
Hide resolved
kanej
left a comment
kanej
left a comment
There was a problem hiding this comment.
I added a test out of completeness.
I have run this PR against the OpenZeppelin connectOnBefore branch to check that it is robust in the face of many network connection creations.
No description provided.