-
Notifications
You must be signed in to change notification settings - Fork 64
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
Compare hashes obtained from local scripts and tenderly simulations against the ones in VALIDATIONS.md #736
base: main
Are you sure you want to change the base?
Conversation
@alcueca Can you provide an example command that I can run this to test? thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first pass
@@ -143,4 +144,35 @@ simulate-verify-task TASK="": | |||
# For testing purposes, we do not gain anything by simulating as other nested safes. | |||
nested_safe_name="foundation" | |||
|
|||
${root_dir}/src/improvements/script/simulate-verify-task.sh {{TASK}} $nested_safe_name | |||
# If TASK doesn't start with / make it absolute using $PWD | |||
if [[ "{{TASK}}" != /* ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is the user expected to pass through an absolute path here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, where is the simulate-verify-task TASK="":
just command expected to be executed from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user can enter a relative or absolute path to the task. If it is relative it is converted to absolute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added comments about when the command is supposed to be used, and what the parameter is
nested_safe_name="foundation" | ||
|
||
# Later this networks list should be dynamically generated from the src/improvements/tasks directory. | ||
for task in ${root_dir}/src/improvements/tasks/example/*/*; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to be simulating non-terminal tasks from the networks directories and not the example directory.
simulate-non-terminal-tasks:
#!/usr/bin/env bash
set -euo pipefail
root_dir=$(git rev-parse --show-toplevel)
forge build
# Later this networks list should be dynamically generated from the src/improvements/tasks directory.
networks=("eth" "sep")
for network in ${networks[@]}; do
if [ "$network" != "src/improvements/tasks/example" ]; then # skip example tasks
for task in ${root_dir}/src/improvements/tasks/${network}/*; do
${root_dir}/src/improvements/script/simulate-verify-task.sh $task $nested_safe_name
done
fi
echo "Done simulating non-terminal tasks for network: $network"
done
```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be put this in a separate PR. The feature being added here is simulating non-terminal tasks in CI for the new superchain-ops repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is that I don't have any tasks outside examples
to test the functionality with, so if I exclude it now it does nothing.
I suggest that we don't put this into CI, and leave it as a developer tool, until we have one or more tasks outside examples
, then we can exclude it from simulate-non-terminal-tasks and put it on CI.
import "forge-std/Script.sol"; | ||
import {GnosisSafeHashes} from "src/libraries/GnosisSafeHashes.sol"; | ||
|
||
contract CalculateSafeHashes is Script { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who calls this? What's the TENDERLY_PAYLOAD
? I know its mentioned in the simulate-verify-task.sh
file below but more information in this file as to what it's purpose is would be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I added a detailed comments about that.
@@ -8,46 +8,73 @@ simulate_verify_task() { | |||
root_dir=$(git rev-parse --show-toplevel) | |||
local_output_file="./simulation_output.txt" | |||
remote_output_file="./remote_output.txt" | |||
forge_output_file="./forge_output.txt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need all the file i/o here with local/remote/forge_output_file? When it fails for whatever reason before cleanup we leave these files lying around for devs to delete manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are scraping the output of other scripts to capture the payload and the hashes.
I tried to capture the output in a variable and scrape that, but we also want the output on console for debugging. It then becomes really hard to capture the output from both stdout, stderr in the variable and always show it regardless of the underlying script exit code.
The current system is more robust, as you always get all the output on the console for debugging. However, I've move the commands to remove files so that they are removed as soon as possible. If a script fails, only one file would remain, which also would have the relevant output if needed. The file would be removed in the next successful execution.
I hope that's a reasonable compromise.
@blmalone, thanks a lot for your comments, and apologies for not putting enough documentation upfront to avoid you getting confused. To try the functionality, you will need API access to Tenderly. You can either: Once you have that sorted out, you can go to From As a question, where would be the best place for me to put this usage info? |
Description
Partially fixes #679.
Pending:
Decide on a format for including hashes in VALIDATIONS.md
Extend scripts to all safes, not just foundation
Exclude examples directory
Add
just simulate-non-terminal-tasks
to CI with tenderly context (for the Tenderly Access Token)