fix: address review findings from #176#179
Merged
lionakhnazarov merged 9 commits intoMay 5, 2026
Merged
Conversation
- Add kind: "transparent" to upgradeProxy call to match upgrade-token-staking.ts and eliminate dependency on OZ manifest for proxy-type detection - Remove unused deployer variable from getNamedAccounts destructuring - Replace sync fs calls with fs.promises equivalents - Wrap JSON.parse in try/catch
Plaintext private keys written to .env.operator-N were world-readable under the default umask. chmod 0600 limits access to the current user.
…ddress Keys passed via --private-key are visible in /proc/<pid>/cmdline and ps aux during execution. Use ETH_PRIVATE_KEY env var instead, which Foundry's cast reads without exposing it in the process table.
The PR introduced a reformatted version that dropped 4 constructor parameters (_keepStakingContract, _nucypherStakingContract, _keepVendingMachine, _keepStake), leaving a 2-param ABI that does not match the deployed contract at 0x01B67b...765dd7. Restore the original 6-param ABI from main so downstream tooling is not broken.
run-new-operator-setup.sh had a cast_send_ok without retry logic; setup-multiple-operators.sh had one with exponential backoff. Nonce-lag errors that were handled transparently in the multi-operator script would fail silently in the single-operator script. Extract the full retry implementation into scripts/lib/cast-helpers.sh and source it from both scripts.
--list was calling fromEncryptedJson with an empty password to extract the address. Keystore JSON contains an unencrypted "address" field; reading it directly avoids spurious decryption errors for keystores with real passwords and removes the dependency on the ethers decrypt path.
- Remove hardcoded fallback T token address; fail loudly with a clear error if T.json is missing rather than silently using a stale address - Quote all shell variables in the cast send call to prevent word splitting on RPC URLs that include API key query parameters
07_deploy_token_staking.ts was writing to both the root TokenStaking.json and deployments/<network>/TokenStaking.json. After the upgrade script runs, only the network-specific file is updated, leaving the root file with a stale ABI. Write only to deployments/<network>/ and delete the root-level artifact that was previously committed. Also fix pre-existing issues in the file: use strict equality, static import for fs, const for jsonAbi, async fs.promises, and try/catch around JSON.parse.
…s.sh to use positional arguments for cast wallet address - Refactored fund-new-operator.sh to utilize cast_send_ok for sending tokens, improving error handling. - Modified setup-multiple-operators.sh to pass private keys as positional arguments to cast wallet address, addressing security concerns with environment variable exposure.
lionakhnazarov
approved these changes
May 5, 2026
lionakhnazarov
left a comment
lionakhnazarov
left a comment
There was a problem hiding this comment.
setup-multiple-operators.sh should derive addresses with cast wallet address or --private-key, not ETH_PRIVATE_KEY=… cast wallet address (Foundry doesn’t wire that env for wallet address). - fixed in following commit
except this - LGTM
lionakhnazarov
merged commit 3ce2a09
into
feat/testnet4-deployment-support
11 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.
Summary
Follow-up to #176. Addresses all P1/P2/P3 findings from the multi-agent review.
P1 fixes:
deploy/54_upgrade_token_staking_extended.ts: addkind: "transparent"toupgradeProxycall; removes reliance on OZ manifest for proxy-type detectionscripts/setup-new-staking-provider.js:chmod 0600env file after write (private keys were world-readable under default umask)scripts/setup-multiple-operators.sh: replacecast wallet address --private-key "$KEY"withETH_PRIVATE_KEY="$KEY" cast wallet addressto avoid key exposure in process tabledeployments/mainnet/TokenStaking.json: restore correct 6-param constructor ABI (PR had accidentally replaced it with a 2-param variant, breaking ABI correctness for the deployed mainnet contract)P2 fixes:
cast_send_okwith retry/backoff intoscripts/lib/cast-helpers.sh; bothrun-new-operator-setup.shandsetup-multiple-operators.shsource it. Previouslyrun-new-operator-setup.shhad no retry logic.scripts/get-operator-key.js:--listmode now reads the unencryptedaddressfield from keystore JSON directly, instead of attempting decryption with an empty passwordscripts/fund-new-operator.sh: remove hardcoded fallback T address (0x60185Ef...); fail loudly with an error ifT.jsonis missing; quote allcast sendvariablesdeploy/07_deploy_token_staking.ts: remove dual-write to rootTokenStaking.json(only write todeployments/<network>/); delete the previously committed root artifactAlso fixed (pre-existing hook findings triggered by touched files):
fs.*Synccalls withfs.promisesin deploy scriptsJSON.parsein try/catch where flagged==with===in network name checksTest plan
cast wallet addressworks withETH_PRIVATE_KEYenv var (Foundry supports it)node scripts/get-operator-key.js --listlists addresses without password errorsdeployments/mainnet/TokenStaking.jsonconstructor shows 6 params matching the live contract