Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically run tasks in CI if task is in a non-terminal state #337

Open
mds1 opened this issue Oct 2, 2024 · 6 comments · May be fixed by #420
Open

Automatically run tasks in CI if task is in a non-terminal state #337

mds1 opened this issue Oct 2, 2024 · 6 comments · May be fixed by #420
Assignees
Labels
good first issue Good for newcomers

Comments

@mds1
Copy link
Contributor

mds1 commented Oct 2, 2024

In script/utils/check-task-statuses.sh we define the following allowed task status:

VALID_STATUSES=("DRAFT, NOT READY TO SIGN" "CONTINGENCY TASK, SIGN AS NEEDED" "READY TO SIGN" "SIGNED" "EXECUTED" "CANCELLED")

Of these, the SIGNED (for presigned pauses only), EXECUTED. and CANCELLED statuses are terminal, meaning once a task's status is changed to this value it should never change again.

When we add new tasks, the task author must manually remember to add them to CI (see just_simulate_sc_rehearsal_1 as an example), but this is easy to forget. Instead, we should have CI do the following:

  • Loop over all tasks in all tasks/* directories.
  • Extract all tasks that have a status that is not a terminal status
  • Determine if they are single safe or nested safe tasks (this can be done based on the name of the Solidity file—if it's NestedSignFromJson it's a nested safe task, otherwise it's a standard single safe)
  • Simulate the task in CI (simulation commands differ for nested vs. standard). For nested safes, we should also simulate the approve calls, which provides an additional mitigation for Wrong state diff checks during nested approval calls #268
  • The CI job should fail if any approval or simuation fails

The reason we filter tasks by "tasks in a non-terminal status" instead of "statues that precede execution" is to be more robust: If a new task status is added, by default CI will try executing tasks with those status, and we can add that task status to the terminal status list if needed

@bordolot
Copy link

bordolot commented Dec 2, 2024

Hey, I'm wondering if I could do that, but I'm not sure, if I'm understanding this task correclty.

Am I right in thinking that the job is to modify ./circleci/config.yml file by adding a new job to the workflow?

The job involves listing all non-terminal tasks, and running a simulation for each, as defined in the README.md file of the task? Is that correct?

@mds1
Copy link
Contributor Author

mds1 commented Dec 4, 2024

Hey! Yes that's close. So you'll see the script/utils/check-task-statuses.sh already loops through all tasks to check their status. We can modify that file (and rename it accordingly, to also simulate). That file is already executed in CI in the check_task_statuses, so we'd modify that existing workflow based on the file rename.

Something like this diff is how I'd approach it. Let me know if you have any more questions, and if you want to go ahead implementing this! Would love to assign the issue to you if you're interested :)

diff --git a/script/utils/check-task-statuses.sh b/script/utils/check-task-statuses.sh
index 89c77c5..4494383 100644
--- a/script/utils/check-task-statuses.sh
+++ b/script/utils/check-task-statuses.sh
@@ -4,6 +4,9 @@ set -euo pipefail
 VALID_STATUSES=("DRAFT, NOT READY TO SIGN" "CONTINGENCY TASK, SIGN AS NEEDED" "READY TO SIGN" "SIGNED" "EXECUTED" "CANCELLED")
 errors=() # We collect all errors then print them at the end.
 
+# Array holding paths of tasks ready to simulate.
+ready_to_simulate=()
+
 # Function to check status and hyperlinks for a single file.
 check_status_and_hyperlinks() {
   local file_path=$1
@@ -40,6 +43,11 @@ check_status_and_hyperlinks() {
       errors+=("Error: Status is EXECUTED but no link to transaction found in $file_path")
     fi
   fi
+
+  # If a file is not in a terminal state, add it to the ready_to_simulate array.
+  if [[ "$status_line" != *"SIGNED"* && "$status_line" != *"EXECUTED"* && "$status_line" != *"CANCELLED"* ]]; then
+    ready_to_simulate+=("$file_path")
+  fi
 }
 
 # Find README.md files for all tasks and process them.
@@ -57,3 +65,16 @@ if [[ ${#errors[@]} -gt 0 ]]; then
 else
   echo "✅ All task statuses are valid"
 fi
+
+# Execute all tasks that are ready to simulate.
+if [[ ${#ready_to_simulate[@]} -gt 0 ]]; then
+  echo "Simulating tasks:"
+  for path in "${ready_to_simulate[@]}"; do
+    echo "  Simulating $path"
+    # Check if the task is nested or single. We can usually do this by looking
+    # at the name of the Solidity file in this directory. If it's SignFromJson.s.sol
+    # it's single, and if it's NestedSignFromJson.s.sol it's nested.
+    # For nested tasks we should sign as both the foundation and the council.
+    # TODO implement the above.
+  done
+fi

@bordolot
Copy link

bordolot commented Dec 4, 2024

Thanks for the answer! ;)
"assign the issue to you if you're interested"? Yeah, I’d like to take it on. Whether I’ll be able to finish it, well, that’s another question XD.

I've already listed all non-terminal tasks. I've made some changes, such as:

  1. Dividing the VALID_STATUSES variable into NON_TERMINAL_STATUSES and TERMINAL_STATUSES
  2. Defining FOLDER_WITH_NO_TASKS - I suppose this is necessary because, without this filter, the list would include folders like:
    ./tasks/sep/fp-recovery/005-set-game-implementation/templates/README.md
    ./tasks/templates/00-default/README.md
    ./tasks/templates/00-protocol-versions/README.md
    Am I correct?There is no need to do simulations for these?

Let’s say I have all the non-terminal tasks, and I want to simulate them. Should there be a universal simulation pattern: one for nested tasks and another for single tasks?

Currently, we have the following (these folders were provided by my listing function):
In the folders:
./tasks/eth/fp-recovery/001-blacklist-game/README.md
./tasks/eth/fp-recovery/002-fallback-permissioned-game/README.md
./tasks/eth/fp-recovery/003-enable-permissionless-game/README.md
./tasks/eth/fp-recovery/004-recover-bonds/README.md
./tasks/sep/fp-recovery/001-blacklist-game/README.md
./tasks/sep/fp-recovery/002-fallback-permissioned-game/README.md
./tasks/sep/fp-recovery/003-enable-permissionless-game/README.md
./tasks/sep/fp-recovery/004-recover-bonds/README.md
./tasks/sep/fp-recovery/005-set-game-implementation/README.md
Should the simulation for these be prepared based on the "Preparing the Operation" section in each respective README.md file? There are no clear guidelines in these files.

In the folder:
./tasks/sep/001-op-extended-pause/README.md
The README.md provides a guideline to use the local justfile and call just simulate.

In the folder:
./tasks/sep/base-003-fp-granite-prestate/README.md
The task simulation is described in ./SINGLE.md.

In the folder:
./tasks/sep/013-fp-granite-prestate/README.md
The task simulation is described in ./NESTED.md.

Am I correct in thinking that ./SINGLE.md and ./NESTED.md contain direct, universal guidelines for single and nested tasks, respectively? Should all subsequent tasks be simulated based on one of these two files?

If so, what should we do with tasks in the following folders: './tasks/eth/fp-recovery/...' ,'./tasks/sep/fp-recovery/...' and './tasks/sep/001-op-extended-pause'?

Current changes I've made:

diff --git a/script/utils/check-task-statuses.sh b/script/utils/check-task-statuses.sh
index 89c77c5..8c09e67 100644
--- a/script/utils/check-task-statuses.sh
+++ b/script/utils/check-task-statuses.sh
@@ -1,9 +1,19 @@
 #!/bin/bash
 set -euo pipefail
 
-VALID_STATUSES=("DRAFT, NOT READY TO SIGN" "CONTINGENCY TASK, SIGN AS NEEDED" "READY TO SIGN" "SIGNED" "EXECUTED" "CANCELLED")
+NON_TERMINAL_STATUSES=("DRAFT, NOT READY TO SIGN" "CONTINGENCY TASK, SIGN AS NEEDED" "READY TO SIGN")
+TERMINAL_STATUSES=("SIGNED" "EXECUTED" "CANCELLED")
+VALID_STATUSES=( "${NON_TERMINAL_STATUSES[@]}" "${TERMINAL_STATUSES[@]}" )
 errors=() # We collect all errors then print them at the end.
 
+# Name of a file to exclude from searching for non-terminal tasks.
+FOLDER_WITH_NO_TASKS="templates"
+# Name of a file in a task directiory that specifies that the task is a nested safe task.
+IF_THIS_ITS_NESTED="NestedSignFromJson.s.sol"
+
+single_tasks_to_simulate=()
+nested_tasks_to_simulate=()
+
 # Function to check status and hyperlinks for a single file.
 check_status_and_hyperlinks() {
   local file_path=$1
@@ -42,6 +52,28 @@ check_status_and_hyperlinks() {
   fi
 }
 
+search_non_terminal_tasks(){
+  local directory
+  for file in $filtered_files; do
+    # Ensure it's a regular file.
+    if [[ -f "$file" ]]; then
+      # Read file content and search for any status in the NON_TERMINAL_STATUSES array.
+      for status in "${NON_TERMINAL_STATUSES[@]}"; do
+        if grep -q "$status" "$file"; then
+          directory=$(dirname "$file")
+          # Specify if a task is safe or nested.
+          if [[ -f "$directory/$IF_THIS_ITS_NESTED" ]]; then
+            nested_tasks_to_simulate+=("$file")
+          else
+            single_tasks_to_simulate+=("$file")
+          fi
+          break
+        fi
+      done
+    fi
+  done
+}
+
 # Find README.md files for all tasks and process them.
 files=$(find ./tasks -type f -path './tasks/*/*/README.md')
 for file in $files; do
@@ -57,3 +89,15 @@ if [[ ${#errors[@]} -gt 0 ]]; then
 else
   echo "✅ All task statuses are valid"
 fi
+
+# Excludes tasks defined in folder FOLDER_WITH_NO_TASKS
+filtered_files=$(echo "$files" | grep -v "/${FOLDER_WITH_NO_TASKS}/")
+search_non_terminal_tasks
+echo "Simulating single tasks..."
+for value in "${single_tasks_to_simulate[@]}"; do
+  echo "$value"
+done
+echo "Simulating nested tasks..."
+for value in "${nested_tasks_to_simulate[@]}"; do
+  echo "$value"
+done

@mds1
Copy link
Contributor Author

mds1 commented Dec 4, 2024

Thank you!

Am I correct in thinking that ./SINGLE.md and ./NESTED.md contain direct, universal guidelines for single and nested tasks, respectively? Should all subsequent tasks be simulated based on one of these two files?

This is correct, for those it should always be something like

# single.md
SIMULATE_WITHOUT_LEDGER=1 just \
   --dotenv-path $(pwd)/.env \
   --justfile ../../../single.just \
   simulate

# nested.md
# for nested, first simulate as council
SIMULATE_WITHOUT_LEDGER=1 just \
   --dotenv-path $(pwd)/.env \
   --justfile ../../../nested.just \
   simulate \    
   council 0

# then simulate as foundation
SIMULATE_WITHOUT_LEDGER=1 just \
   --dotenv-path $(pwd)/.env \
   --justfile ../../../nested.just \
   simulate \    
   foundation 0

Do you mind opening a draft PR and we can continue discussions there? That would make it easier to stay organized :)

For ./tasks/sep/001-op-extended-pause/README.md, ./tasks/sep/base-003-fp-granite-prestate/README.md, and ./tasks/sep/013-fp-granite-prestate/README.md—these actually were already executed. I will go back and look for the transaction hashes to correct these in a separate PR

bordolot added a commit to bordolot/superchain-ops that referenced this issue Dec 6, 2024
bordolot added a commit to bordolot/superchain-ops that referenced this issue Dec 6, 2024
@bordolot
Copy link

bordolot commented Dec 6, 2024

A draft PR is ready.

@mds1
Copy link
Contributor Author

mds1 commented Dec 16, 2024

Comment from @maurelian:

Do we have plans to run them sequentially in the same forked EVM state to avoid nonce issues?

This is a good idea, since some tasks are dependent on previous tasks, and something we should think about how to best support in a follow up PR

blmalone pushed a commit that referenced this issue Dec 19, 2024
blmalone pushed a commit that referenced this issue Dec 19, 2024
blmalone pushed a commit that referenced this issue Dec 19, 2024
blmalone pushed a commit that referenced this issue Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Development

Successfully merging a pull request may close this issue.

2 participants