Support safe Sepolia staking upgrades and operator setup - #176
lionakhnazarov wants to merge 44 commits into
Conversation
…rease in TokenStaking - Introduced method in and to allow governance to approve applications. - Added method to enable staking providers to increase their authorization for applications. - Updated deployment scripts for TokenStaking upgrade and added new scripts for operator keystore management. - Modified to include a new script for upgrading token staking. - Created new JSON file for TokenStaking ABI and removed outdated deployment data.
lrsaturnino
left a comment
lrsaturnino
left a comment
There was a problem hiding this comment.
Two critical findings from multi-model code review (3/3 models confirmed both).
- Updated the approveApplication function to include the onlyGovernance modifier, ensuring that only governance can approve applications.
… for Yarn - Upgraded actions/setup-node from v3 to v4 across multiple workflow files. - Added a step to enable Corepack for Yarn in the workflows, ensuring compatibility with the package manager specified in the project.
…ld-network/solidity-contracts into feat/testnet4-deployment-support
- Removed comments from .yarnrc.yml to streamline configuration. - Updated packageManager in package.json to Yarn version 4.12.0 for improved performance and features.
…docs file - Changed the workflow to use the local path for reusable-solidity-docs.yml instead of the remote repository reference. - Ensured that both documentation generation and publishing jobs point to the correct local file.
- Remove private key logging from create-operator-keystore and get-operator-key; address is sufficient for confirmation - Remove staking provider and operator private keys from generated .env files; keys written to disk are a git-leak risk - Print staking provider key once to terminal with a prominent "copy now" warning instead of persisting it to .env - Require non-empty password in create-operator-keystore and setup-new-staking-provider; empty-password keystores are trivially decryptable - Require explicit keystore path in get-operator-key; remove the hardcoded developer-machine UUID default that caused ENOENT for all other users - Fix --list path in get-operator-key: ../../operator-1-keystore resolved above repo root; corrected to ../operator-1-keystore - Convert sync fs calls to fs.promises and add try/catch inside main() across all three scripts
…args Passing --private-key as a CLI argument exposes the key in ps aux output and persists in shell history. Using ETH_PRIVATE_KEY as an inline env assignment (ETH_PRIVATE_KEY="$key" cast send ...) keeps the key out of the argv list. Introduce _sp_cast_send_ok / _op_cast_send_ok wrappers in run-new-operator-setup.sh that inject ETH_PRIVATE_KEY for the respective signer, and replace all --private-key flag usages. Update fund-new-operator.sh likewise for the deployer key. Update run-new-operator-setup.sh usage comment to reflect that NEW_STAKING_PROVIDER_KEY and NEW_OPERATOR_KEY are no longer written to .env files and must be exported by the operator.
- Drop kind: "transparent" from upgradeProxy options; let the OZ
plugin infer the proxy type from the deployed proxy admin slot.
Hardcoding the kind risks a mismatch if the original deploy
defaulted differently. Add a comment with the cast storage
command to verify proxy type on-chain.
- Replace two inline const fs = require("fs") declarations with a
single top-level import * as fs from "fs" to match TypeScript
conventions and avoid the duplicate binding.
The file was removed in the parent branch commit without explanation. Downstream consumers relying on deployments/mainnet/TokenStaking.json break silently without it. Restored from the last known-good version (commit ab29e02).
approveApplication already checks application != address(0) but increaseAuthorization did not. The APPROVED status check provides a functional backstop, but adding the explicit guard makes the invariant consistent across both entry points.
…and-correctness fix: security and correctness follow-ups for Sepolia operator tooling (PR #176)
lrsaturnino
left a comment
lrsaturnino
left a comment
There was a problem hiding this comment.
Looks good overall — clean contract changes, CEI ordering solid, access controls in place.
One small nit: the operator setup scripts generate keystores and env files (operator-1-keystore/, spv-maintainer-keystore/, .env.new-operator, .env.operator-*) that aren't covered by .gitignore. Might be worth adding entries to prevent accidental commits.
- Added new entries to .gitignore for generated operator setup artifacts. - Updated deployment scripts to create a directory for network-specific deployments and save TokenStaking deployment data in both the root and network-specific directories. - Refactored authorization and registration commands in setup scripts to use environment variables for private keys, improving security and readability. - Modified upgrade script documentation to reflect the correct command usage from the repository root.
…ld-network/solidity-contracts into feat/testnet4-deployment-support
lrsaturnino
left a comment
lrsaturnino
left a comment
There was a problem hiding this comment.
On ExtendedTokenStaking in contracts/test/TokenStakingTestSet.sol — now that approveApplication and increaseAuthorization live on the base TokenStaking, the overrides at lines 227–268 and 280–294 are duplicates. The increaseAuthorization override also doesn't emit AuthorizationIncreased or guard application != address(0), while the base does both.
Since deploy/54 makes ExtendedTokenStaking the live Sepolia implementation, Sepolia will silently stop emitting AuthorizationIncreased (no test asserts it, so CI won't flag it). Dropping both overrides should be enough — the derived contract picks up the canonical behaviour from the base.
- Introduced a function to remove CRLF and whitespace from environment variables, preventing decoding errors. - Updated to ensure that the is not overwritten by stale values. - Modified to store both staking provider and operator private keys in the environment file for automated setups, while ensuring sensitive information is not logged unnecessarily. - Added error handling for missing keys in the generated environment files.
- Added validation to ensure ETH_PRIVATE_KEY is set before sending transactions. - Introduced a mechanism to prevent overwriting the deployer key with stale values from environment files. - Updated the script to maintain the correct private key for the contract owner during operator setup.
- Updated prerequisites for deploying operators to include AUTO_FUND_T for automatic minting of T tokens. - Added a function to compute T token shortfall and validate the deployer's balance. - Implemented error handling for insufficient T balance and ensured proper private key management for minting. - Introduced normalization for addresses to improve consistency in key comparisons.
--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.
…llowup fix: address review findings from #176
cast send and cast wallet address do not honor ETH_PRIVATE_KEY as an environment variable, so passing the key via --private-key (or as a positional argument to cast wallet address) leaves it visible in /proc/<pid>/cmdline and `ps auxww` output for every subprocess. Move signing through a v3 keystore + password file: - scripts/lib/wallet.js: read ETH_PRIVATE_KEY from env (never argv), derive the address, or write an encrypted v3 keystore. Foundry expects lower-case "crypto"; ethers v5 emits "Crypto", so the field name is rewritten before persisting. - scripts/lib/cast-helpers.sh: lazy-create a per-process keystore directory (mode 700, in $TMPDIR), encrypt each unique key on first use, cache by derived address, remove the directory via an EXIT trap that chains onto any existing trap. cast_send_ok now calls cast send with --keystore + --password-file. derive_address_safe replaces the `cast wallet address <KEY>` callers. - scripts/setup-multiple-operators.sh: switch the three cast wallet address callsites to derive_address_safe. Threat model is process argv exposure on multi-tenant runners; the keystore lives only for the lifetime of the sourcing shell and is never persisted across invocations.
The previous commit moved signing through a v3 keystore so the key
stopped appearing in argv (visible to all users via `ps auxww`). The
temporary env assignment used by callers,
ETH_PRIVATE_KEY="$KEY" cast_send_ok ...
is still propagated into every child process spawned inside
cast_send_ok, including the `cast send` invocation, so the key remained
readable from /proc/<cast-pid>/environ for the lifetime of the
subprocess (same-user/root only on Linux, but nonzero).
cast send and cast receipt sign / inspect via --keystore + --password-file
at this point and do not need the variable. Wrap both invocations with
`env -u ETH_PRIVATE_KEY` so the variable is dropped from their
environment. The wallet.js subprocesses still inherit it because they
read ETH_PRIVATE_KEY from env to derive the address and to encrypt the
keystore.
|
Probably a quick follow-up — commit the manifest (from whoever ran |
Add a .gitignore exception for unknown-11155111.json so the OpenZeppelin manifest is versioned like mainnet layout history, avoiding "Manifest not found" on fresh clones when running Sepolia upgrades. Co-authored-by: Cursor <cursoragent@cursor.com>
…llowup chore: track OpenZeppelin Sepolia upgrade manifest
mswilkison
changed the title
|
Addressed all seven review findings in 0fab89a:
Validation: 410 tests passed (30 new), compilation, packaging, local deployment/export, repository-pinned Slither 0.8.0 (0 findings), modified-file lint/format, and Bash syntax. Storage upgrade checks cover both the base contract and previous fixture layout. Independent candidate review also identified local redeployment and failed-generation retry regressions; both are corrected and included in the tests. These are source fixes; no Sepolia or mainnet deployment was performed. GitHub CI on 0fab89a is green: build/tests, deployment dry run, Slither, formatting workflow, and documentation preview all passed. |
|
Fixed the three follow-up findings in a8fe18c.
Changed files: Verification, in order:
No public-network deployment or live subgraph validation was performed. The downstream checks use tBTC's dependency versions in isolated local consumers; they do not deploy the entire tBTC system. GitHub CI results will be recorded separately. |
|
Fixed the CI installation failure in a7ba9ac. The failed run stopped while installing dependencies: concurrent Yarn Classic bootstraps for Git dependencies shared the runner's cache and hit an The Solidity, format, npm and reusable docs workflows now pass Verified that two concurrent Yarn Classic 1.22.22 processes resolve different cache directories, and that all four workflows parse correctly. All checks passed on this commit: Solidity (build/tests, deployment dry run and Slither), format, and documentation preview. |
|
Addressed both new findings in 3620f4d.
The manifest recovery is based on read-only Sepolia evidence at block 11663843. Recompiling historical commit Verification:
The repository-wide Compatibility: OpenZeppelin 1.14 has one admin per manifest. The active entry now matches the currently recorded Sepolia proxy; older proxies with the historical admin require their matching historical checkout/manifest. External consumers retain their own manifest and must incorporate the recovered history as documented. No validation bypass or automatic manifest overwrite was added. GitHub CI passed for head |
|
Addressed both operator-tooling findings in 594bf49.
Added 19 regression cases in Verification, in order:
The previously reported repository-wide formatting errors in unchanged keystore scripts are outside this patch; changed-file checks pass. No public-chain deployment or transfer was performed. GitHub CI passed for head |
4 participants
Sepolia operators need native T staking and authorization for Random Beacon and Wallet Registry. This adds that workflow through a dedicated
SepoliaTokenStakingimplementation, with proxy deployment, upgrade, funding, keystore, and operator registration tooling.Stakedevents for indexers, keeps every approved application's authorization backed during decreases and withdrawals, and disables legacy TACo migration on Sepolia. The base contract retains TACo eligibility and rejects deprecated applications before changing authorization.external.contracts. The package includes matching compiler build information, which the consuming project's OpenZeppelin plugin validates in its own cache format before deploying. Missing or mismatched build information stops the operation before a transaction; existing consumer validation entries remain intact.ExtendedTokenStakinglayout, includingskipList. Its implementation bytecode was rebuilt from the deployment commit and matched against Sepolia. The manifest is included in npm distributions; upgrade-history documentation explains provenance and consumer/historical-admin requirements.deployments.save, so later operations and exports includestake()..env.operator-*and plural.env.operators-*credential files are ignored.PATHfrom selecting another provider while preserving each command's working-directory behavior.setup-multiple-operators.shuses the same normalized decimal wei amount for the ETH balance check and both wallet transfers. UnitlessETH_PER_OPERATORvalues are interpreted as ether, so0.05and1fund each wallet with the intended amount.Validation on the remediation commits:
npm run prepack(including Solidity compilation, TypeScript and artifact/build-info export) andtsc -p tsconfig.export.json --noEmitpassed.hardhat deploy --network hardhat --export /tmp/pr176-followup-export.jsonpassed.The broader
npm run formatcommand still reports 58 pre-existing style errors in three unchanged keystore scripts; the changed JavaScript and shell files pass the applicable lint, formatting, and syntax checks.No public-network deployment was performed. Coordinate the shared deployment-script changes with #186 and the toolchain refresh in #182 when integrating these PRs.