cleanup: remove TestBed and duplicate SimpleTest mock plumbing#246
cleanup: remove TestBed and duplicate SimpleTest mock plumbing#246alexshchur wants to merge 30 commits intomasterfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
🦋 Changeset detectedLatest commit: 67c7be3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
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 |
…nto tests/cleanup
alexshchur
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
architect-dev
left a comment
architect-dev
left a comment
There was a problem hiding this comment.
In the hardhat-plugin-test package we're using the SimpleTest by fetching the artifact from the test-setup, but I think we should be able to import the contract directly and deploy it with the standard hardhat deployment pipeline. I think how its set up in this PR means that it will always be type any since we no longer have the typechain coming from mock-contracts. Is this a real issue or is it already handled?
Most everything looks great, a few small changes I've noted though.
| "@fhenixprotocol/cofhe-contracts/=node_modules/@fhenixprotocol/cofhe-contracts/", | ||
| "@cofhe/mock-contracts/=node_modules/@cofhe/mock-contracts/" | ||
| "@cofhe/mock-contracts/=node_modules/@cofhe/mock-contracts/", | ||
| "@cofhe/test-setup/=../../test/setup/" |
There was a problem hiding this comment.
Maybe better to import the @cofhe/test-setup package and pull the contract from node_modules? This feels somewhat brittle to me
| const fixedContracts = { | ||
| MockTaskManager: TASK_MANAGER_ADDRESS, | ||
| MockZkVerifier: MOCKS_ZK_VERIFIER_ADDRESS, | ||
| MockThresholdNetwork: MOCKS_THRESHOLD_NETWORK_ADDRESS, | ||
| TestBed: TEST_BED_ADDRESS, | ||
| MockTaskManager: { | ||
| fixedAddress: TASK_MANAGER_ADDRESS, | ||
| inspectTarget: 'MockTaskManager', | ||
| }, | ||
| MockZkVerifier: { | ||
| fixedAddress: MOCKS_ZK_VERIFIER_ADDRESS, | ||
| inspectTarget: 'MockZkVerifier', | ||
| }, | ||
| MockThresholdNetwork: { | ||
| fixedAddress: MOCKS_THRESHOLD_NETWORK_ADDRESS, | ||
| inspectTarget: 'MockThresholdNetwork', | ||
| }, | ||
| }; | ||
|
|
||
| const nonFixedContracts = { | ||
| MockACL: true, | ||
| MockACL: { | ||
| inspectTarget: 'MockACL', | ||
| }, | ||
| }; |
There was a problem hiding this comment.
If we're switching to use the inspectTarget (which I like by the way), then this should be cleaned up. Instead of a keyed object lets just use an array Array<{ fixedAddress?: Hex, inspectTarget: string }>. If fixed address is populated then we do a fixed address deployment, if its undefined we'll do a variable address deployment.
| @@ -52,8 +57,8 @@ export const ${name}Artifact = ${JSON.stringify(artifact, null, 2)} as const sat | |||
There was a problem hiding this comment.
Lets extract this out into a fn writeFixedAddressArtifact, and below lines 63-77 extract out writeVariableAddressArtifact.
|
|
||
| for (const [name, fixedAddress] of Object.entries(fixedContracts)) { | ||
| const abi = inspect(name, 'abi'); | ||
| for (const [name, { fixedAddress, inspectTarget }] of Object.entries(fixedContracts)) { |
There was a problem hiding this comment.
With a single array of contracts to deploy we don't need to do Object.entries anymore
| import { describe, it, beforeEach } from 'node:test'; | ||
| import assert from 'node:assert/strict'; | ||
| import { network } from 'hardhat'; | ||
| import SimpleTestArtifact from '../../setup/out/SimpleTest.sol/SimpleTest.json'; |
There was a problem hiding this comment.
Should be able to import from @cofhe/test-setup right?
…nto tests/cleanup
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 75502f1. Configure here.
2 participants
Remove the remaining legacy
TestBedsurface and clean up duplicatedSimpleTestplumbing across the mock stack.The main change is that the Hardhat integrations now only auto-deploy the core mock contracts.
TestBed/SimpleTestis no longer treated as a special runtime mock and should be deployed explicitly by tests that need it.What changed
TestBedfrom@cofhe/mock-contractsgetTestBed()/TestBedmock access from the Hardhat pluginsdeployTestBed/ auto-deployment ofSimpleTestfrom the Hardhat mock deployment flowTEST_BED_ADDRESSfrom@cofhe/sdkSimpleTestwrapper/export from@cofhe/mock-contractsSimpleTestexplicitly from the shared fixture/artifact where neededBreaking changes
hre.cofhe.mocks.getTestBed()is gonecofhe.mocks.TestBeddeployTestBedSimpleTestis no longer auto-deployed as part of the mock environmentTEST_BED_ADDRESSis no longer exported from@cofhe/sdk@cofhe/mock-contractsno longer ships the duplicateSimpleTestwrapper/artifact surfaceNote
Medium Risk
Medium risk because it removes exported constants/types and changes Hardhat plugin APIs/behavior (no
TestBeddescriptor, nodeployTestBedoption), which can break existing tests and integrations expecting auto-deployed helper contracts.Overview
Removes the legacy
TestBedhelper contract surface across the mock stack and makes Hardhat integrations auto-deploy only the core mock contracts.Hardhat v2 and v3 plugins drop
TestBeddeployment/injection (deployTestBedoption,cofhe.mocks.TestBed, andhre.cofhe.mocks.getTestBed()), and docs/tests are updated to deploySimpleTestexplicitly from@cofhe/test-setupartifacts when needed.@cofhe/mock-contractsstops shipping/exportingTestBedartifacts/types (and related generated files),@cofhe/sdkremovesTEST_BED_ADDRESS, and Foundry + integration test setups add@cofhe/test-setupremappings/dependencies to use the sharedSimpleTestfixture.Reviewed by Cursor Bugbot for commit 67c7be3. Bugbot is set up for automated code reviews on this repo. Configure here.