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

Use array to create deployment command #753

Merged
merged 1 commit into from
Dec 22, 2023

Conversation

smol-ninja
Copy link
Member

@smol-ninja smol-ninja commented Dec 18, 2023

Closes #750

@smol-ninja smol-ninja force-pushed the fix/CREATE2-salt-interpolation branch from 8e77f64 to 36b2c7f Compare December 19, 2023 14:04
@smol-ninja smol-ninja merged commit 8827002 into build/multi-chain-script Dec 22, 2023
9 checks passed
Copy link
Member

@andreivladbrg andreivladbrg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you able to run the script anymore?

Later update: this fails only when running with --deterministic flag

image image

Although I have my MNEMONIC set up correctly in both .env .env.deployment but it still uses the default one:

https://github.com/sablier-labs/v2-core/blob/c1cca898090f76b869a42853cb7d5b4b29d8a679/script/Base.s.sol#L7-L8

It might be from multiple environments.

I have an idea, why aren't we changing the scripts from script/ to pass 2 uint256 for chain id and version instead of a string since it is this problematic.

@andreivladbrg
Copy link
Member

Forgot to add this, I suggest we change with:

    # Choose the script based on the flag
    if [[ $DETERMINISTIC_DEPLOYMENT == true ]]; then
        echo -e "\n${IC}Deploying deterministic contracts to $chain...${NC}"
        # Construct the command as an array
        deployment_command=("forge" "script" "script/DeployDeterministicCore3.s.sol")
        deployment_command+=("--rpc-url" "$rpc_url")
        deployment_command+=("--sig" "run(string,address,address,uint256)")
        deployment_command+=("ChainID ${chain_id}, Version 1.1.1")
        deployment_command+=("$admin")
        deployment_command+=("$comptroller")
        deployment_command+=("$MAX_SEGMENT_COUNT")
        deployment_command+=("-vvv")
    else
        echo -e "\n${IC}Deploying contracts to $chain...${NC}"
        # Construct the command as an array
        deployment_command=("forge" "script" "script/DeployCore3.s.sol")
        deployment_command+=("--rpc-url" "$rpc_url")
        deployment_command+=("--sig" "run(address,address,uint256)")
        deployment_command+=("$admin")
        deployment_command+=("$comptroller")
        deployment_command+=("$MAX_SEGMENT_COUNT")
        deployment_command+=("-vvv")
    fi
  
   FOUNDRY_PROFILE=optimized "${deployment_command[@]}"

Each part of the command is an element in the array. This helps in keeping arguments that contain spaces grouped correctly and when expanding the array to run the command ("${deployment_command[@]}"), each element of the array is treated as a separate argument, preserving the intended structure of the command.

@andreivladbrg
Copy link
Member

I have an idea, why aren't we changing the scripts from script/ to pass 2 uint256 for chain id and version instead of a string since it is this problematic.

I know that version has this format "1.1.1" but we can make something to work

@smol-ninja
Copy link
Member Author

Although I have my MNEMONIC set up correctly in both .env .env.deployment but it still uses the default one

I have seen this error. This is because you may be trying to simulate to deploy the deterministic address on a forked chain where it already exists, for example Sepolia where I deployed them already while testing (Now I think thats a problem and should be avoided). But if you try other chains, it should work.

I have an idea, why aren't we changing the scripts from script/ to pass 2 uint256 for chain id and version instead of a string since it is this problematic.

The problem is to be able to construct the expected deployment_command through script because of how it interprets string. I don't see how passing chain id and version as uint256 solves this. chain id is still being passed as an argument to the command. Did I understand you correctly?

Forgot to add this, I suggest we change with

Agree. Fixed in #765.

@andreivladbrg
Copy link
Member

andreivladbrg commented Dec 22, 2023

I have seen this error. This is because you may be trying to simulate to deploy the deterministic address on a forked chain where it already exists
for example Sepolia where I deployed them already while testing

yeah, but we are now using a new compiler 0.8.23 with an older "Version 1.1.1" and the bytecodes differ (the version used on contracts for 0.8.23 is "Version 1.1.2"), i didn't know that you've already deployed using "Version 1.1.1". it makes sense now, thanks

The problem is to be able to construct the expected deployment_command through script because of how it interprets string. I don't see how passing chain id and version as uint256 solves this. chain id is still being passed as an argument to the command. Did I understand you correctly?

Yes, the problem is to construct the expected deployment_command. Passing the chain id and the version as uint256 would solve this because you wouldn't have " or ' in the CLI:

With string:

FOUNDRY_PROFILE=optimized \
forge script script/DeployDeterministicLockupLinear.s.sol \
--rpc-url $SEPOLIA_RPC_URL \
--sig "run(string,address,address,address)" \
"ChainID 11155111, Version 1.1.2" \
0x79Fb3e81aAc012c08501f41296CCC145a1E15844 \
0xC3Be6BffAeab7B297c03383B4254aa3Af2b9a5BA \
0x23eD5DA55AF4286c0dE55fAcb414dEE2e317F4CB
-vvvv

vs

With uint256:

FOUNDRY_PROFILE=optimized \
forge script script/DeployDeterministicLockupLinear.s.sol \
--rpc-url $SEPOLIA_RPC_URL \
--sig "run(uint256,uint256,address,address,address)" \
11155111 \ -> chainid
4 \ -> version
0x79Fb3e81aAc012c08501f41296CCC145a1E15844 \
0xC3Be6BffAeab7B297c03383B4254aa3Af2b9a5BA \
0x23eD5DA55AF4286c0dE55fAcb414dEE2e317F4CB
-vvvv

@smol-ninja
Copy link
Member Author

Yes, the problem is to construct the expected deployment_command. Passing the chain id and the version as uint256 would solve this because you wouldn't have " or ' in the CLI:

I agree, this could be an alternate solution to this. I think what we have now is good enough. It works as it should be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants