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

Update main.star #2019

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
149 changes: 85 additions & 64 deletions testing/forge-script/main.star
Original file line number Diff line number Diff line change
@@ -1,119 +1,140 @@
import shlex

SOURCE_DIR_PATH = "/app/contracts"
IMAGE_FOUNDRY = "ghcr.io/foundry-rs/foundry:latest"
ENTRYPOINT = ["/bin/sh"]
DEPENDENCY_DIR_PATH = "/app/dependency"
OUTPUT_FILE = "output.json"
CONTRACTS_DIR = "contracts"
DEPENDENCY_DIR = "dependency"
TRANSACTION_FILE_CMD = "grep 'Transactions saved to' output.json | awk -F': ' '{print $2}'"
testBaseFee = 123

def run(plan, deployment = {}):
def run(plan, deployment={}):
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid mutable default arguments

In the run function, using a mutable default argument like {} for deployment can lead to unexpected behaviors because the default mutable object is shared across all calls to the function.

Apply this diff to fix the issue:

-def run(plan, deployment={}):
+def run(plan, deployment=None):
+    if deployment is None:
+        deployment = {}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def run(plan, deployment={}):
def run(plan, deployment=None):
if deployment is None:
deployment = {}

deploy_contracts(plan, deployment)

# Define the function to run the Forge script for deployment
def deploy_contracts(plan, deployment):
repository = deployment["repository"]
contracts_path = deployment["contracts_path"]
script_path = deployment["script_path"]
contract_name = deployment["contract_name"]
rpc_url = deployment["rpc_url"]
wallet = deployment["wallet"]
dependency = deployment["dependency"]
dependency_type = dependency["type"]

# TODO: Support other wallet options such as mnemonics, keystore, hardware wallets.
if wallet["type"] == "private_key":
wallet_command = "--private-key {}".format(wallet["value"])
# Function to run commands and check for errors
def run_command(plan, service_name, command, error_message):
result = plan.exec(
service_name=service_name,
recipe=ExecRecipe(
command=["/bin/sh", "-c", command],
),
)
if result["code"] != 0:
fail(error_message)
return result

# Get the wallet command
def get_wallet_command(wallet):
wallet_type = wallet["type"]
if wallet_type == "private_key":
return "--private-key {}".format(shlex.quote(wallet["value"]))
else:
Comment on lines +31 to 33
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling for missing wallet["value"]

The code assumes that wallet["value"] exists, which may raise a KeyError if the key is missing. It's important to check for the presence of this key before using it.

Add a check to ensure wallet["value"] is present:

 def get_wallet_command(wallet):
     wallet_type = wallet["type"]
     if wallet_type == "private_key":
+        if "value" not in wallet:
+            fail("Wallet value not specified for private_key type")
         return "--private-key {}".format(shlex.quote(wallet["value"]))
     else:
         fail("Wallet type {} not supported.".format(wallet_type))
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if wallet_type == "private_key":
return "--private-key {}".format(shlex.quote(wallet["value"]))
else:
if wallet_type == "private_key":
if "value" not in wallet:
fail("Wallet value not specified for private_key type")
return "--private-key {}".format(shlex.quote(wallet["value"]))
else:

fail("Wallet type {} not supported.".format(wallet["type"]))
fail("Wallet type {} not supported.".format(wallet_type))

folder = plan.upload_files(src = repository, name = "contracts")
# Main function for deploying contracts
def deploy_contracts(plan, deployment):
# Fetching data from deployment with checks
repository = deployment.get("repository")
if not repository:
fail("Repository not specified in deployment")
contracts_path = deployment.get("contracts_path", "")
script_path = deployment.get("script_path", "")
contract_name = deployment.get("contract_name", "")
rpc_url = shlex.quote(deployment.get("rpc_url", ""))
Comment on lines +44 to +45
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Check for missing contract_name and rpc_url before use

The contract_name and rpc_url are retrieved with default empty strings but used in commands. If either is empty, the command may fail or behave unexpectedly.

Add checks to ensure contract_name and rpc_url are provided:

 contract_name = deployment.get("contract_name", "")
+if not contract_name:
+    fail("Contract name not specified in deployment")
 rpc_url = shlex.quote(deployment.get("rpc_url", ""))
+if not deployment.get("rpc_url"):
+    fail("RPC URL not specified in deployment")
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
contract_name = deployment.get("contract_name", "")
rpc_url = shlex.quote(deployment.get("rpc_url", ""))
contract_name = deployment.get("contract_name", "")
if not contract_name:
fail("Contract name not specified in deployment")
rpc_url = shlex.quote(deployment.get("rpc_url", ""))
if not deployment.get("rpc_url"):
fail("RPC URL not specified in deployment")

wallet = deployment.get("wallet")
if not wallet:
fail("Wallet not specified")
dependency = deployment.get("dependency", {})
dependency_type = dependency.get("type", "")

wallet_command = get_wallet_command(wallet)

folder = plan.upload_files(src=repository, name=CONTRACTS_DIR)

dependency_artifact_name = ""
if dependency_type == "local" or dependency_type == "git":
dependency_path = dependency["path"]

plan.upload_files(src = "dependency", name = "dependency")
dependency_artifact_name = "dependency"
dependency_path = dependency.get("path", "")
plan.upload_files(src=DEPENDENCY_DIR, name=DEPENDENCY_DIR)
Comment on lines +58 to +59
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Check for missing dependency_path before use

The dependency_path is retrieved with a default empty string but used in commands. If it's empty, the command may fail.

Add a check to ensure dependency_path is provided when a dependency is specified:

 dependency_path = dependency.get("path", "")
+if not dependency_path:
+    fail("Dependency path not specified for dependency type '{}'".format(dependency_type))
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
dependency_path = dependency.get("path", "")
plan.upload_files(src=DEPENDENCY_DIR, name=DEPENDENCY_DIR)
dependency_path = dependency.get("path", "")
if not dependency_path:
fail("Dependency path not specified for dependency type '{}'".format(dependency_type))
plan.upload_files(src=DEPENDENCY_DIR, name=DEPENDENCY_DIR)

dependency_artifact_name = DEPENDENCY_DIR

foundry_service = plan.add_service(
name = "foundry",
config = get_service_config(dependency_artifact_name),
name="foundry",
config=get_service_config(dependency_artifact_name),
)

if contracts_path:
contract_path = "{}/{}".format(SOURCE_DIR_PATH, contracts_path)
else:
contract_path = SOURCE_DIR_PATH
contract_path = "{}/{}".format(SOURCE_DIR_PATH, contracts_path) if contracts_path else SOURCE_DIR_PATH

if dependency_type == "local":
# Run shell script
plan.exec(
service_name = foundry_service.name,
recipe = ExecRecipe(
command = ["/bin/sh", "-c", "sh {}/{}".format(DEPENDENCY_DIR_PATH, dependency_path)],
),
)
run_command(plan, foundry_service.name, "sh {}/{}".format(DEPENDENCY_DIR_PATH, dependency_path), "Local dependency execution failed")
elif dependency_type == "git":
plan.exec(
service_name = foundry_service.name,
recipe = ExecRecipe(
command = ["/bin/sh", "-c", "cd {} && sh {}".format(contract_path, dependency_path)],
),
)
run_command(plan, foundry_service.name, "cd {} && sh {}".format(contract_path, dependency_path), "Git dependency execution failed")
Comment on lines +70 to +72
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Secure dependency_path to prevent command injection

The dependency_path variable is used in shell commands without proper sanitization. This could lead to command injection vulnerabilities if dependency_path contains malicious input.

Apply shlex.quote to dependency_path when constructing shell commands:

 if dependency_type == "local":
-    run_command(plan, foundry_service.name, "sh {}/{}".format(DEPENDENCY_DIR_PATH, dependency_path), "Local dependency execution failed")
+    run_command(plan, foundry_service.name, "sh {}/{}".format(DEPENDENCY_DIR_PATH, shlex.quote(dependency_path)), "Local dependency execution failed")
 elif dependency_type == "git":
-    run_command(plan, foundry_service.name, "cd {} && sh {}".format(contract_path, dependency_path), "Git dependency execution failed")
+    run_command(plan, foundry_service.name, "cd {} && sh {}".format(contract_path, shlex.quote(dependency_path)), "Git dependency execution failed")

Committable suggestion was skipped due to low confidence.


if script_path:
result = plan.exec(
service_name = foundry_service.name,
recipe = ExecRecipe(
command = ["/bin/sh", "-c", "cd {} && forge build".format(contract_path)],
),
)
plan.verify(result["code"], "==", 0)
# Logging the build and deployment process
plan.print("Running deployment script: {}".format(script_path))
run_command(plan, foundry_service.name, "cd {} && forge build".format(contract_path), "Forge build failed")
Comment on lines +67 to +77
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Sanitize contracts_path when used in shell commands

The contracts_path variable is derived from deployment data and used in shell commands without proper sanitization. This could lead to command injection vulnerabilities.

Apply shlex.quote to contracts_path when constructing shell commands:

 contract_path = "{}/{}".format(SOURCE_DIR_PATH, contracts_path) if contracts_path else SOURCE_DIR_PATH

+contract_path_quoted = shlex.quote(contract_path)

-    run_command(plan, foundry_service.name, "cd {} && forge build".format(contract_path), "Forge build failed")
+    run_command(plan, foundry_service.name, "cd {} && forge build".format(contract_path_quoted), "Forge build failed")

 ...

-        "cd {} && {}".format(contract_path, TRANSACTION_FILE_CMD)
+        "cd {} && {}".format(contract_path_quoted, TRANSACTION_FILE_CMD)

 ...

-    "cat {}/{}".format(contract_path, OUTPUT_FILE),
+    "cat {}/{}".format(contract_path_quoted, OUTPUT_FILE),

Also applies to: 83-89, 96-96, 105-105

Comment on lines +75 to +77
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Sanitize script_path and contract_name to prevent command injection

The script_path and contract_name variables are used in shell commands without sanitization. This could lead to vulnerabilities if they contain malicious input.

Apply shlex.quote to script_path and contract_name when constructing shell commands:

 plan.print("Running deployment script: {}".format(script_path))
+script_path_quoted = shlex.quote(script_path)
+contract_name_quoted = shlex.quote(contract_name)
 
-    run_command(plan, foundry_service.name, "cd {} && forge build".format(contract_path), "Forge build failed")
+    run_command(plan, foundry_service.name, "cd {} && forge build".format(contract_path_quoted), "Forge build failed")

 ...

     script_output = exec_on_service(
         plan,
         foundry_service.name,
-        "cd {} && forge script {}:{} --broadcast --rpc-url {} {} --json --skip test > {}".format(
-            contract_path,
-            script_path,
-            contract_name,
+        "cd {} && forge script {}:{} --broadcast --rpc-url {} {} --json --skip test > {}".format(
+            contract_path_quoted,
+            script_path_quoted,
+            contract_name_quoted,
             rpc_url,
             wallet_command,
             OUTPUT_FILE
         ),
     )

Also applies to: 83-89


# Executing forge script
script_output = exec_on_service(
plan,
foundry_service.name,
"cd {} && forge script {}:{} --broadcast --rpc-url {} {} --json --skip test > output.json ".format(
"cd {} && forge script {}:{} --broadcast --rpc-url {} {} --json --skip test > {}".format(
contract_path,
script_path,
contract_name,
rpc_url,
wallet_command,
OUTPUT_FILE
Comment on lines +83 to +89
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve readability by refactoring command construction

The command string used in the forge script execution is lengthy and complex. Refactoring it can enhance readability and maintainability.

Refactor the command construction as follows:

script_command = "cd {} && forge script {}:{} --broadcast --rpc-url {} {} --json --skip test > {}".format(
    contract_path_quoted,
    script_path_quoted,
    contract_name_quoted,
    rpc_url,
    wallet_command,
    OUTPUT_FILE
)

# Then use the command in exec_on_service or run_command
run_command(
    plan,
    foundry_service.name,
    script_command,
    "Forge script execution failed",
)

),
)

exec_on_service(
plan,
foundry_service.name,
"cat {}/output.json ".format(contract_path),
"cat {}/{}".format(contract_path, OUTPUT_FILE),
)

if script_path:
# Get the forge script output in a output.json file and grep from it
transaction_file = "grep 'Transactions saved to' output.json | awk -F': ' '{print $2}'"
plan.print("transaction_file", transaction_file)

transaction_file_details = exec_on_service(plan, foundry_service.name, "cd {} && {}".format(contract_path, transaction_file))

# Fetch and check the transaction file
plan.print("Retrieving transaction file...")
transaction_file_details = exec_on_service(
plan,
foundry_service.name,
"cd {} && {}".format(contract_path, TRANSACTION_FILE_CMD)
)

if not transaction_file_details["output"]:
fail("Transaction file not found.")
exec_output = exec_on_service(plan, foundry_service.name, "chmod -R 777 /app/contracts && cat {}".format(transaction_file_details["output"]))

exec_output = exec_on_service(
plan,
foundry_service.name,
"chmod -R 777 /app/contracts && cat {}".format(transaction_file_details["output"])
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Sanitize transaction_file_details["output"] to prevent command injection

The transaction_file_details["output"] is used in a shell command without sanitization. This could be exploited if the output contains malicious content.

Apply shlex.quote to transaction_file_details["output"] when constructing shell commands:

 exec_output = exec_on_service(
     plan,
     foundry_service.name,
-    "chmod -R 777 /app/contracts && cat {}".format(transaction_file_details["output"])
+    "chmod -R 777 /app/contracts && cat {}".format(shlex.quote(transaction_file_details["output"]))
 )

Committable suggestion was skipped due to low confidence.

)
Comment on lines +79 to +115
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Unify command execution and error handling

Multiple commands are executed using exec_on_service without consistent error handling. To maintain consistency and ensure proper error handling, consider using run_command instead of exec_on_service where appropriate.

Refactor the code to use run_command for command execution that requires error checks:

-# Executing forge script
-script_output = exec_on_service(
-    plan,
-    foundry_service.name,
-    "cd {} && forge script ...".format(...),
-)
+run_command(
+    plan,
+    foundry_service.name,
+    "cd {} && forge script ...".format(...),
+    "Forge script execution failed",
+)

 ...

-transaction_file_details = exec_on_service(
-    plan,
-    foundry_service.name,
-    "cd {} && {}".format(contract_path_quoted, TRANSACTION_FILE_CMD)
-)
+transaction_file_details = run_command(
+    plan,
+    foundry_service.name,
+    "cd {} && {}".format(contract_path_quoted, TRANSACTION_FILE_CMD),
+    "Transaction file retrieval failed",
+)

 ...

-exec_output = exec_on_service(
-    plan,
-    foundry_service.name,
-    "chmod -R 777 /app/contracts && cat {}".format(shlex.quote(transaction_file_details["output"]))
-)
+exec_output = run_command(
+    plan,
+    foundry_service.name,
+    "chmod -R 777 /app/contracts && cat {}".format(shlex.quote(transaction_file_details["output"])),
+    "Failed to read transaction file",
+)

Committable suggestion was skipped due to low confidence.

plan.verify(exec_output["code"], "==", 0)

# Function to execute commands inside the service
def exec_on_service(plan, service_name, command):
return plan.exec(
service_name = service_name,
recipe = ExecRecipe(
command = ["/bin/sh", "-c", command],
service_name=service_name,
recipe=ExecRecipe(
command=["/bin/sh", "-c", command],
),
)

def get_service_config(dependency_artifact_name = None):
# Service configuration
def get_service_config(dependency_artifact_name=None):
files = {
SOURCE_DIR_PATH: "contracts",
SOURCE_DIR_PATH: CONTRACTS_DIR,
}

if dependency_artifact_name:
files[DEPENDENCY_DIR_PATH] = dependency_artifact_name

return ServiceConfig(
image = IMAGE_FOUNDRY,
entrypoint = ENTRYPOINT,
files = files,
image=IMAGE_FOUNDRY,
entrypoint=ENTRYPOINT,
files=files,
)