review: address PR #4000 review findings (deploy hardening + type-safety)#4016
Merged
lionakhnazarov merged 7 commits intoJun 4, 2026
Conversation
…ator Invert mainnet-only denylist to explicit allowlist (hardhat, development, sepolia). Future production-like networks added downstream now default to the safe behavior (preserve existing deployment) instead of silently overwriting deployments/<network>/EcdsaDkgValidator.json.
Add README explaining that 05_approve_random_beacon_in_token_staking.js intentionally diverges from its tsc-compiled siblings to carry an ifaceHasFunction precheck for Threshold TokenStaking. A blind regeneration from upstream would silently drop the precheck and reintroduce a hard failure on networks without approveApplication.
The conditional etherscan spread inside the object literal forced removal of the HardhatUserConfig annotation. Assign etherscan via a post-declaration if-block instead so the annotation (and type-checking) is preserved without relying on TS 4.9 'satisfies' (this package is on TS 4.5).
Sepolia maps deployer/governance/chaosnetOwner/esdm all to account index 0 because the testnet deploy uses a single key from ACCOUNTS_PRIVATE_KEYS. Add a header comment so readers don't expect role-separation branches (e.g. initialize-wallet-owner.ts's owner-vs-governance fork) to fire on Sepolia.
The second WalletRegistry.governance() read immediately before execute() is a deliberate recheck against concurrent transferGovernance on shared networks (sepolia/mainnet); annotate so it isn't mistaken for dead code.
Add verifyOnTenderlyOrContinue mirroring verifyOnEtherscanOrContinue and wrap tenderly.verify in all four deploy scripts (01, 02, 03, 09). Before this, only 03 swallowed Tenderly errors; a Tenderly outage during deploy would fail 01/02/09 while 03 kept going. Now all four behave the same.
hardhat-deploy walks deploy/ and requires() every file; a Markdown sibling there crashes deployments.fixture() in tests. Move the regeneration-policy README one level up to external/random-beacon-export/README.md (still discoverable, no longer in the require path).
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
lionakhnazarov
approved these changes
Jun 4, 2026
lionakhnazarov
merged commit 3f5e3c9
into
stack/testnet4-02-solidity-logic
31 checks passed
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.
Context
Follow-up to #4000 addressing valid findings from the multi-agent review. Stacks on top of
stack/testnet4-02-solidity-logic; merge after #4000.Findings addressed
EcdsaDkgValidator(was: mainnet-only denylist)random-beacon-exportregeneration policy (05_*.jsdiverges intentionally)Findings rejected after review
WalletRegistry.sol(NatSpec-only) — best handled by editing PR feat(solidity): improve Sepolia deploy behavior and wallet registry flows #4000's body directly, not as a code commit here.Test Plan
Verified locally with `FORKING_URL` unset:
The 44 + 397 pending are the pre-existing `describe.skip(...)` suites (legacy Keep TokenStaking ABI unavailable in current Threshold build).