WIP: SBIN Code Refactor

config/e2e_test/.paths.env

@@ -6,7 +6,7 @@ MAGI_DIR=$ROOT_DIR/services/cl_clients/magi
 SIDECAR_DIR=$ROOT_DIR/services/sidecar
 # Define binaries
 OPS_GENESIS_BIN=$OPS_DIR/build/bin/genesis
-L1_GETH_BIN=$GETH_DIR/build/bin/geth # TODO: use l1, not sp?
+L1_GETH_BIN=$GETH_DIR/build/bin/geth # TODO(#299): use l1, not sp?
 SP_GETH_BIN=$GETH_DIR/build/bin/geth
 SP_MAGI_BIN=$MAGI_DIR/target/debug/magi
 SIDECAR_BIN=$SIDECAR_DIR/build/bin/sidecar

config/example/.paths.env

@@ -11,7 +11,7 @@ SIDECAR_DIR=$ROOT_DIR/services/sidecar
 
 # Define binaries
 OPS_GENESIS_BIN=$OPS_DIR/build/bin/genesis
-L1_GETH_BIN=$GETH_DIR/build/bin/geth # TODO: use l1, not sp?
+L1_GETH_BIN=$GETH_DIR/build/bin/geth # TODO(#300): use l1, not sp?
 SP_GETH_BIN=$GETH_DIR/build/bin/geth
 SP_MAGI_BIN=$MAGI_DIR/target/debug/magi
 SIDECAR_BIN=$SIDECAR_DIR/build/bin/sidecar

config/local_devnet/.paths.env

@@ -6,7 +6,7 @@ MAGI_DIR=$ROOT_DIR/services/cl_clients/magi
 SIDECAR_DIR=$ROOT_DIR/services/sidecar
 # Define binaries
 OPS_GENESIS_BIN=$OPS_DIR/build/bin/genesis
-L1_GETH_BIN=$GETH_DIR/build/bin/geth # TODO: use l1, not sp?
+L1_GETH_BIN=$GETH_DIR/build/bin/geth # TODO(#301): use l1, not sp?
 SP_GETH_BIN=$GETH_DIR/build/bin/geth
 SP_MAGI_BIN=$MAGI_DIR/target/debug/magi
 SIDECAR_BIN=$SIDECAR_DIR/build/bin/sidecar

contracts/deploy/l1/02_deploy_Rollup.ts

@@ -17,7 +17,7 @@ const func: DeployFunction = async function (hre: HardhatRuntimeEnvironment) {
     vault: process.env.DEPLOYER_ADDRESS,
     daProvider: sequencerInboxProxyAddress,
     verifier: verifierProxyAddress,
-    confirmationPeriod: 12, // TODO: move to config
+    confirmationPeriod: 12, // TODO(#302): move to config
     challengePeriod: 0,
     minimumAssertionPeriod: 0,
     baseStakeAmount: 0,

contracts/scripts/config/create_config.ts

@@ -4,7 +4,7 @@ import { parseFlag, numberStrToPaddedHex } from "./utils";
 
 require("dotenv").config();
 
-// TODO: consider moving to golang (ops).
+// TODO(#303): consider moving to golang (ops).
 async function main() {
   const baseConfigPath = parseFlag("--in");
   const configPath = parseFlag("--out");

contracts/scripts/config/create_deployments_config.ts

@@ -3,7 +3,7 @@ import { parseFlag } from "./utils";
 
 require("dotenv").config();
 
-// TODO: consider moving to golang (ops).
+// TODO(#304): consider moving to golang (ops).
 async function main() {
   const deploymentsConfig = parseFlag("--deployments-config-path");
   const deploymentsPath = parseFlag("--deployments", "./deployments/localhost");

contracts/scripts/e2e/bridge/test_standard_bridge_deposit_eth.ts

@@ -18,7 +18,7 @@ async function main() {
     l1Oracle,
   } = await getSignersAndContracts();
 
-  // TODO: portal should be funded as part of pre-deploy pipeline
+  // TODO(#305): portal should be funded as part of pre-deploy pipeline
   const donateTx = await l2Portal.donateETH({
     value: ethers.utils.parseEther("1"),
   });

contracts/scripts/e2e/bridge/test_standard_bridge_erc20.ts

@@ -27,7 +27,7 @@ async function main() {
   const { l1Token, l2Token } = await deployTokenPair(l1Bridger, l2Relayer);
   console.log(`Deployed tokens ${l1Token.address}, ${l2Token.address}`);
 
-  // TODO: portal should be funded as part of pre-deploy pipeline
+  // TODO(#306): portal should be funded as part of pre-deploy pipeline
   await Promise.all([
     l1Portal.donateETH({ value: ethers.utils.parseEther("1") }),
     l2Portal.donateETH({ value: ethers.utils.parseEther("1") }),

contracts/scripts/e2e/bridge/test_standard_bridge_withdraw_eth.ts

@@ -16,7 +16,7 @@ async function main() {
     rollup,
   } = await getSignersAndContracts();
 
-  // TODO: portal should be funded as part of deploy pipeline
+  // TODO(#307): portal should be funded as part of deploy pipeline
   const donateTx = await l1Portal.donateETH({
     value: ethers.utils.parseEther("1"),
   });

contracts/scripts/e2e/test_transactions.ts

@@ -14,7 +14,7 @@ async function main() {
     l2BaseFeeRecipient: await l2Provider.getBalance(l2BaseFeeRecipient),
   };
 
-  // TODO: should we randomize numTx and value?
+  // TODO(#308): should we randomize numTx and value?
   const numTx = 5;
   for (let i = 0; i < numTx; i++) {
     const tx = await l2Relayer.sendTransaction({
@@ -38,7 +38,7 @@ async function main() {
     throw `balance after transaction does not match the transaction amount on L2Bridge`;
   }
 
-  // TODO: more precise check
+  // TODO(#309): more precise check
   if (!endBalances.l1FeeRecipient.gt(startBalances.l1FeeRecipient)) {
     console.log({ startBalances, endBalances, totalValue });
     throw "did not collect L1 fee";

contracts/src/IRollup.sol

@@ -35,7 +35,7 @@ interface IRollup {
 
     event StakerStaked(address stakerAddr, uint256 assertionID);
 
-    // TODO: Include errors thrown in function documentation.
+    // TODO(#310): Include errors thrown in function documentation.
 
     /// @dev Thrown when the new config parameter is invalid (configuration methods).
     error InvalidConfigChange();
@@ -285,7 +285,7 @@ interface IRollup {
      * @notice Advances msg.sender's existing stake to assertionID.
      * @notice Only callable by whitelisted validators.
      * @param assertionID ID of assertion to advance stake to. Currently this must be a child of the current assertion.
-     * TODO: generalize to arbitrary descendants.
+     * TODO(#311): generalize to arbitrary descendants.
      */
     function advanceStake(uint256 assertionID) external;
 

contracts/src/Rollup.sol

@@ -413,7 +413,7 @@ contract Rollup is RollupBase {
         if (assertionID <= staker.assertionID || assertionID > lastCreatedAssertionID) {
             revert AssertionOutOfRange();
         }
-        // TODO: allow arbitrary descendant of current staked assertionID, not just child.
+        // TODO(#312): allow arbitrary descendant of current staked assertionID, not just child.
         if (staker.assertionID != assertions[assertionID].parent) {
             revert ParentAssertionUnstaked();
         }
@@ -498,7 +498,7 @@ contract Rollup is RollupBase {
         requireUnchallengedStaker(defender);
         requireUnchallengedStaker(challenger);
 
-        // TODO: Calculate upper limit for allowed node proposal time.
+        // TODO(#313): Calculate upper limit for allowed node proposal time.
 
         // Initialize challenge.
         SymChallenge challenge = new SymChallenge();
@@ -651,7 +651,7 @@ contract Rollup is RollupBase {
     }
 
     function newAssertionDeadline() private view returns (uint256) {
-        // TODO: account for prev assertion, gas
+        // TODO(#314): account for prev assertion, gas
         return block.number + confirmationPeriod;
     }
 

contracts/src/bridge/L2Portal.sol

@@ -50,7 +50,7 @@ contract L2Portal is
     /**
      * @notice Address of the L2Portal deployed on L1.
      */
-    address public l1PortalAddress; // TODO: store the hash instead
+    address public l1PortalAddress; // TODO(#315): store the hash instead
 
     /**
      * @notice Address of the L1 account which initiated a deposit in this transaction. If the
@@ -148,7 +148,7 @@ contract L2Portal is
         bytes[] calldata depositAccountProof,
         bytes[] calldata depositProof
     ) external whenNotPaused {
-        // TODO: re-add `onlyProxy`
+        // TODO(#316): re-add `onlyProxy`
         // Prevent nested deposits within deposits.
         require(l1Sender == DEFAULT_L1_SENDER, "L2Portal: can only trigger one deposit per transaction");
 

contracts/src/challenge/SymChallenge.sol

@@ -101,7 +101,7 @@ contract SymChallenge is ChallengeBase, ISymChallenge {
         challengerTimeLeft = challengePeriod;
     }
 
-    // TODO: Clean up challenger/defender turn taking if possible
+    // TODO(#317): Clean up challenger/defender turn taking if possible
     function initializeChallengeLength(uint256 _numSteps) external {
         if (block.number - lastMoveBlock > currentResponderTimeLeft()) {
             revert DeadlineExpired();
@@ -190,7 +190,7 @@ contract SymChallenge is ChallengeBase, ISymChallenge {
         emit Bisected(bisectionHash, challengedSegmentStart, challengedSegmentLength);
     }
 
-    // TODO: If bisection is not executed because both have commited to the same hash at the same `numStep`, verify
+    // TODO(#318): If bisection is not executed because both have commited to the same hash at the same `numStep`, verify
     // there are no additional ops on the stack after the last step.
     function verifyOneStepProof(
         bytes calldata oneStepProof,

sbin/clean.sh

@@ -1,16 +1,23 @@
 #!/bin/bash
-SBIN=$(dirname $0)
-$SBIN/clean_sp_geth.sh
-$SBIN/clean_deployment.sh
 
-echo "Removing dotenv files..."
+set -e
+
+# Get the directory of the script
+script_dir=$(dirname $0)
+
+# Clean up Ethereum related scripts
+$script_dir/clean_sp_geth.sh
+$script_dir/clean_deployment.sh
+
+# Remove dotenv files
 rm -f .contracts.env
 rm -f .genesis.env
 rm -f .sp_geth.env
 rm -f .sp_magi.env
 rm -f .sidecar.env
 rm -f .paths.env
-echo "Done."
+echo "Done removing dotenv files."
 
+# Remove JWT secret file
 echo "Removing $JWT_SECRET_PATH"
 rm -f $JWT_SECRET_PATH

sbin/clean_deployment.sh

@@ -1,38 +1,40 @@
 #!/bin/bash
-SBIN=$(dirname "$(readlink -f "$0")")
-ROOT_DIR=$SBIN/..
 
+set -e
+
+# Get the current script's directory
+SCRIPT_DIR=$(dirname "$(readlink -f "$0")")
+ROOT_DIR=$SCRIPT_DIR/..
+
+# Load environment variables from .paths.env
 PATHS_ENV=".paths.env"
 if ! test -f "$PATHS_ENV"; then
-  echo "Expected dotenv at $PATHS_ENV (does not exist)."
+  echo "Error: $PATHS_ENV not found"
   exit
 fi
 echo "Using dotenv: $PATHS_ENV"
 . $PATHS_ENV
 
+# Load environment variables from .genesis.env if it exists
 GENESIS_ENV=".genesis.env"
 if test -f "$GENESIS_ENV"; then
   . $GENESIS_ENV
 fi
 
-if test -f "$GENESIS_PATH"; then
-  echo "Removing $GENESIS_PATH"
-  rm $GENESIS_PATH
-fi
-if test -f "$GENESIS_EXPORTED_HASH_PATH"; then
-  echo "Removing $GENESIS_EXPORTED_HASH_PATH"
-  rm $GENESIS_EXPORTED_HASH_PATH
-fi
-if test -f "$ROLLUP_CFG_PATH"; then
-  echo "Removing $ROLLUP_CFG_PATH"
-  rm $ROLLUP_CFG_PATH
-fi
-DEPLOYMENTS_ENV=".deployments.env"
-if test -f "$DEPLOYMENTS_ENV"; then
-  echo "Removing $DEPLOYMENTS_ENV"
-  rm $DEPLOYMENTS_ENV
-fi
+# Clean up existing files if they exist
+cleanup_file() {
+  if test -f "$1"; then
+    echo "Removing $1"
+    rm $1
+  fi
+}
+
+cleanup_file "$GENESIS_PATH"
+cleanup_file "$GENESIS_EXPORTED_HASH_PATH"
+cleanup_file "$ROLLUP_CFG_PATH"
+cleanup_file "$DEPLOYMENTS_ENV"
 
+# Remove deployment files and .deployed file
 echo "Removing deployment files in $CONTRACTS_DIR/deployments/$L1_NETWORK"
 rm -rf $CONTRACTS_DIR/deployments/$L1_NETWORK
 rm -f .deployed

sbin/clean_sp_geth.sh

@@ -1,16 +1,18 @@
 #!/bin/bash
+
+set -e
+
 # Check that a dotenv exists.
-GETH_ENV=".sp_geth.env"
-if ! test -f "$GETH_ENV"; then
-  echo "expected dotenv at ./$GETH_ENV (does not exist); could not clean cwd."
+dotenv_file=".sp_geth.env"
+if ! test -f "$dotenv_file"; then
+  echo "Error: expected dotenv at ./$dotenv_file (does not exist); could not clean current working directory."
   exit
 fi
-. $GETH_ENV
-echo "Removing sp-geth data dir $DATA_DIR"
+source $dotenv_file
+echo "Removing sp-geth data directory: $DATA_DIR"
 rm -rf $DATA_DIR
 
 if test -f .start_sp_geth.sh.lock; then
-
+  lock_file=".start_sp_geth.sh.lock"
   echo "Removing docker lock file"
-  L1_WAIT=$WAIT_DIR/.start_sp_geth.sh.lock
 fi

sbin/create_genesis.sh

@@ -1,6 +1,7 @@
 #!/bin/bash
 set -e
-# Currently the local sbin paths are relative to the project root.
+
+# Set the SBIN variable to the directory of the script
 SBIN=$(dirname "$(readlink -f "$0")")
 SBIN="$(
   cd "$SBIN"
@@ -9,21 +10,26 @@ SBIN="$(
 . $SBIN/utils/utils.sh
 ROOT_DIR=$SBIN/..
 
-# Check that the all required dotenv files exists.
-reqdotenv "paths" ".paths.env"
-reqdotenv "genesis" ".genesis.env"
-reqdotenv "contracts" ".contracts.env"
-reqdotenv "deployments" ".deployments.env"
+# Check that all required dotenv files exist
+require_dotenv "paths" ".paths.env"
+require_dotenv "genesis" ".genesis.env"
+require_dotenv "contracts" ".contracts.env"
+require_dotenv "deployments" ".deployments.env"
 
+# Output the ops directory being used
 echo "Using $OPS_DIR as ops directory."
+
 # Get relative paths for $OPS_DIR
-GENESIS_CFG_PATH=$(relpath $GENESIS_CFG_PATH $OPS_DIR)
-GENESIS_PATH=$(relpath $GENESIS_PATH $OPS_DIR)
-GENESIS_EXPORTED_HASH_PATH=$(relpath $GENESIS_EXPORTED_HASH_PATH $OPS_DIR)
+GENESIS_CFG_PATH=$(relative_path $GENESIS_CFG_PATH $OPS_DIR)
+GENESIS_PATH=$(relative_path $GENESIS_PATH $OPS_DIR)
+GENESIS_EXPORTED_HASH_PATH=$(relative_path $GENESIS_EXPORTED_HASH_PATH $OPS_DIR)
+
+# Generate new genesis file and export hash
 echo "Generating new genesis file at $GENESIS_PATH and exporting hash to $GENESIS_EXPORTED_HASH_PATH"
 cd $OPS_DIR
-guard_overwrite $GENESIS_PATH $AUTO_ACCEPT
-# Create genesis.json file.
+confirm_overwrite $GENESIS_PATH $AUTO_ACCEPT
+
+# Create genesis.json file
 FLAGS=(
   "--genesis-config $GENESIS_CFG_PATH"
   "--out $GENESIS_PATH"
@@ -34,7 +40,7 @@ FLAGS=(
   "--alloc $SEQUENCER_ADDRESS,$VALIDATOR_ADDRESS,$DEPLOYER_ADDRESS"
 )
 
-# hoop: I don't have the patience rn to determine why this isn't being sourced
+# Choose the correct genesis binary and run it
 if [[ -z ${OPS_GENESIS_BIN+x} ]]; then
   CMD="/usr/local/bin/genesis ${FLAGS[@]}"
 else
@@ -44,14 +50,14 @@ fi
 echo "Running $CMD"
 eval $CMD
 
-# Initialize a reference to the config files at
-# "contracts/.genesis" (using relative paths as appropriate).
+# Initialize a reference to the config files at "contracts/.genesis" using relative paths as appropriate
 CONTRACTS_ENV=$CONTRACTS_DIR/.genesis.env
-guard_overwrite $CONTRACTS_ENV $AUTO_ACCEPT
-# Write file, using relative paths.
+confirm_overwrite $CONTRACTS_ENV $AUTO_ACCEPT
+
+# Write file using relative paths
 echo "Initializing contracts genesis dotenv $CONTRACTS_ENV"
-GENESIS_PATH=$(relpath $GENESIS_PATH $CONTRACTS_DIR)
-GENESIS_EXPORTED_HASH_PATH=$(relpath $GENESIS_EXPORTED_HASH_PATH $CONTRACTS_DIR)
-BASE_ROLLUP_CFG_PATH=$(relpath $BASE_ROLLUP_CFG_PATH $CONTRACTS_DIR)
+GENESIS_PATH=$(relative_path $GENESIS_PATH $CONTRACTS_DIR)
+GENESIS_EXPORTED_HASH_PATH=$(relative_path $GENESIS_EXPORTED_HASH_PATH $CONTRACTS_DIR)
+BASE_ROLLUP_CFG_PATH=$(relative_path $BASE_ROLLUP_CFG_PATH $CONTRACTS_DIR)
 echo GENESIS_PATH=$GENESIS_PATH >>$CONTRACTS_ENV
 echo GENESIS_EXPORTED_HASH_PATH=$GENESIS_EXPORTED_HASH_PATH >>$CONTRACTS_ENV