Deployment Review Script #683
Conversation
remedcu
force-pushed
the
main
branch
3 times, most recently
from
d357bf2
to
6b63d37
Compare
| fileLineChangeJSON=$(gh pr view $pr --json files) # This line fetches the JSON output of the files changed in the PR | ||
| edgeCase=0 | ||
| echo "$fileLineChangeJSON" | jq -r '.files[] | "\(.path) \(.additions) \(.deletions)"' | while read -r line; do | ||
| path=$(echo $line | cut -d ' ' -f1) |
There was a problem hiding this comment.
uber-nano-nit: If path has a space, this will break. I suggest using a non-standard character (\0 is commonly used).
Not a huge deal for our case, since we know the file names - but just wanted to point it out in case it is easy to fix (if it isn't - leave it as is).
There was a problem hiding this comment.
Did you mean something like:
path=$(echo $line | cut -d '\0' -f1)
In this case, as you mentioned, as file names are known, this might not be a scenario for us.
| # Adding three elements at a time together to compare from the output array | ||
| diffPatch=() | ||
| for ((i=0; i<${#diffPatchSeparated[@]}; i+=3)); do | ||
| diffPatch+=("${diffPatchSeparated[i]} ${diffPatchSeparated[i+1]} ${diffPatchSeparated[i+2]}") |
There was a problem hiding this comment.
Doesn't this depend on how many before lines and after lines are included in the diff?
There was a problem hiding this comment.
Yes, but we already know that if it enters this if branch, there are only 2 additions and 1 subtraction. With grep we only take the actual lines added and deleted in the diff patch. And if it doesn't follow the intended pattern (based on the check we have written), then it errors out, as it should be.
| done | ||
|
|
||
| if [[ $edgeCase == 1 ]]; then | ||
| echo "Edge case when adding chain with the highest chain id number" |
There was a problem hiding this comment.
This loop is quite complicated to me, and I'm not sure I understand it. I will try with fresh eyes on Monday.
| echo "ERROR: "$file" code hash is not the same as the one created for the chain id" 1>&2 | ||
| exit 1 | ||
| fi | ||
| for deploymentType in "${deploymentTypes[@]}"; do |
There was a problem hiding this comment.
Doesn't this run for each version type for each file for each file? My understanding is that the deploymentTypes is built for all files (so it will have a duplicate for each file).
There was a problem hiding this comment.
Yes, it will run for each version if there is more than one version deployed for that particular chain (only limited to v1.3.1). Which I think should not be a problem, other than if the RPC have some limit of less than 18 requests per minute or so.
There was a problem hiding this comment.
This can be fixed by deduping deploymentTypes. My point is that, AFAIU from how JQ works with multiple files specified, for 1.3.1 it will be something like:
deploymentTypes=(canonical eip155 canonical eip155 canonical eip155 canonical eip155 canonical eip155 canonical eip155 canonical eip155 canonical eip155 canonical eip155)
There was a problem hiding this comment.
So, I just checked with the below bash code for this PR:
deploymentTypes=($(jq -r --arg c "$chainid" '[.networkAddresses[$c]] | flatten | .[]' "$versionFiles"))
echo ${deploymentTypes[@]}
for file in "${versionFiles[@]}"; do
for deploymentType in "${deploymentTypes[@]}"; do
echo $deploymentType
And I get the output:
Checking changes to other files
Checking changes to assets files
Verifying Deployment Asset
canonical eip155
canonical
eip155
canonical
eip155
canonical
eip155
canonical
eip155
canonical
eip155
canonical
eip155
canonical
eip155
canonical
eip155
canonical
eip155
Network addresses & Code hashes are correct
This is what I was expecting, so it doesn't have 18 values in total for each file, instead has 2 values (based on the deployment type: canonical and eip155) for each file.
There was a problem hiding this comment.
Oh... I guess I don't know bash arrays very well. Is "$versionFiles" implicitly doing "${versionFiles[0]}"?
There was a problem hiding this comment.
I believe yes: https://stackoverflow.com/a/47368011/7520013
|
Test results:
In all the PR's the |
|
|
||
| # Getting default addresses, address on the chain and checking code hash. | ||
| deploymentTypes=($(jq -r --arg c "$chainid" '[.networkAddresses[$c]] | flatten | .[]' "$versionFiles")) |
There was a problem hiding this comment.
I think this is dangerous for arrays - you should use:
| deploymentTypes=($(jq -r --arg c "$chainid" '[.networkAddresses[$c]] | flatten | .[]' "$versionFiles")) | |
| deploymentTypes=($(jq -r --arg c "$chainid" '[.networkAddresses[$c]] | flatten | .[]' "${versionFiles[@]}")) |
There was a problem hiding this comment.
Now this comment make more sense. So, instead of all the version files, I am intentionally only selecting a single file (defaults to the first value in the array), as the deployment types should be the same for all the files.
So, if I make it to parse the values from all files (based on the above suggestion), then it will return all the deployment types of all the files, which is not the intended output.
| }, | ||
| "eip155": { | ||
| "address": "0x998739BFdAAdde7C933B942a68053933098f9EDa", | ||
| "codeHash": "0x81db0e4afdf5178583537b58c5ad403bd47a4ac7f9bde2442ef3e341d433126a" |
There was a problem hiding this comment.
Interesting that the codehash is different here 🤔
There was a problem hiding this comment.
The codeHash for multiSend and SimulateTxAccessor is different as it uses the address(this) within the constructor, thus making a different bytecode based on the address.
This PR improves the review cycle for the
safe-deploymentby checking the new deployments when a new PR is created for a particular chain.This PR:
ISSUE_TEMPLATE, introduced a PR Template which will ask for the required data from the PR creator, and help run the CI script.github-reviewscript, which checks for edge cases when a chain ID with the highest number is added.codehash(becausev.1.3.0has multiple deployment types).Remaining TODO's:
v1.3.0) from the current list. This can't be detected with the current script.