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.

Sign up for GitHub

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

Avax deployers #112

Merged
merged 2 commits into from
Dec 16, 2024
Merged

Avax deployers #112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1484521
avax deployers
aazhou1 Dec 16, 2024
18e9659
avax deployers
aazhou1 Dec 16, 2024
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
85 changes: 85 additions & 0 deletions .github/workflows/deploy-avax-strategy.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
name: "[avalanche-deploy] deploy strategy"
on:
workflow_dispatch:
inputs:
asset:
description: 'Asset address'
required: true
default: '0x'
strategyName:
description: 'Yearn strategy name'
required: true
default: '0x'
strategyManagementAddress:
description: 'Strategy management address'
required: true
default: '0x'
discountRateMarkup:
description: 'Discount rate markup'
required: false
default: '0.01'
timeToMaturityThreshold:
description: 'Time to maturity threshold'
required: false
default: '0'
repoTokenConcentrationLimit:
description: 'Repo token concentration limit'
required: false
default: '0.01'
requiredReserveRatio:
description: 'Required reserve ratio'
required: false
default: '0.01'
profitMaxUnlock:
description: 'Profit max unlock time'
required: false
default: '0'
collateralTokens:
description: 'Collateral tokens comma separated'
required: false
default: '0x'
minCollateralRatios:
description: 'Minimum collateral ratio comma separated'
required: false
default: '0.01'

jobs:
deploy:
runs-on: ubuntu-latest
environment:
name: avalanche
url: https://term-finance.github.io/yearn-v3-term-vault/
Comment on lines +47 to +51
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Documentation URL is not accessible

The environment URL https://term-finance.github.io/yearn-v3-term-vault/ returns a 404 error, indicating that the GitHub Pages documentation site is either not published or incorrectly configured. Please ensure that:

  • The GitHub Pages site is properly set up for the repository
  • The documentation is published to the correct branch
  • The URL path is accurate
🔗 Analysis chain

Verify the documentation URL accessibility.

The environment URL points to a GitHub Pages documentation site. Please ensure this documentation is publicly accessible and contains relevant deployment information.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the documentation URL is accessible
curl -I https://term-finance.github.io/yearn-v3-term-vault/

Length of output: 1063

🧰 Tools
🪛 yamllint (1.35.1)

[error] 49-49: trailing spaces

(trailing-spaces)

steps:
- uses: actions/checkout@master
with:
fetch-depth: 0
submodules: recursive
- uses: foundry-rs/foundry-toolchain@v1
- run: forge install
- run: forge build
- run: forge tree
- run: forge script script/Strategy.s.sol:DeployStrategy --rpc-url $RPC_URL --broadcast --gas-price 500000000000 --verify --verbosity 4
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix shell script quoting for security.

The forge script command should have its arguments properly quoted to prevent word splitting and potential injection issues.

-      - run: forge script script/Strategy.s.sol:DeployStrategy --rpc-url $RPC_URL --broadcast --gas-price  500000000000  --verify --verbosity 4
+      - run: forge script "script/Strategy.s.sol:DeployStrategy" --rpc-url "$RPC_URL" --broadcast --gas-price "500000000000" --verify --verbosity 4
📝 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
- run: forge script script/Strategy.s.sol:DeployStrategy --rpc-url $RPC_URL --broadcast --gas-price 500000000000 --verify --verbosity 4
- run: forge script "script/Strategy.s.sol:DeployStrategy" --rpc-url "$RPC_URL" --broadcast --gas-price "500000000000" --verify --verbosity 4
🧰 Tools
🪛 actionlint (1.7.4)

61-61: shellcheck reported issue in this script: SC2086:info:1:61: Double quote to prevent globbing and word splitting

(shellcheck)

env:
RPC_URL: ${{ secrets.RPC_URL }}
PRIVATE_KEY: ${{ secrets.PRIVATE_KEY }}
GOVERNOR_DEPLOYER_KEY: ${{ secrets.GOVERNANCE_DEPLOYER_KEY }}
ETHERSCAN_API_KEY: ${{ secrets.ETHERSCAN_API_KEY }}
ASSET_ADDRESS: ${{ github.event.inputs.asset }}
YEARN_VAULT_ADDRESS: ${{ vars.YEARN_VAULT_ADDRESS }}
IS_TEST: true
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove or justify IS_TEST=true in production deployment.

Setting IS_TEST=true in a production deployment workflow is concerning. This might bypass important safety checks.

Please either:

  1. Remove this environment variable for production deployments
  2. Document why it's necessary to have it set to true

STRATEGY_NAME: ${{ github.event.inputs.strategyName }}
TERM_CONTROLLER_ADDRESS: ${{ vars.TERM_CONTROLLER_ADDRESS }}
DISCOUNT_RATE_ADAPTER_ADDRESS: ${{ vars.DISCOUNT_RATE_ADAPTER_ADDRESS }}
DISCOUNT_RATE_MARKUP: ${{ github.event.inputs.discountRateMarkup }}
TIME_TO_MATURITY_THRESHOLD: ${{ github.event.inputs.timeToMaturityThreshold }}
REPOTOKEN_CONCENTRATION_LIMIT: ${{ github.event.inputs.repoTokenConcentrationLimit }}
ADMIN_ADDRESS: ${{ vars.ADMIN_ADDRESS }}
DEVOPS_ADDRESS: ${{ vars.DEVOPS_ADDRESS }}
KEEPER_ADDRESS: ${{ vars.KEEPER }}
GOVERNOR_ROLE_ADDRESS: ${{ vars.GOVERNANCE_FACTORY }}
STRATEGY_MANAGEMENT_ADDRESS: ${{ github.event.inputs.strategyManagementAddress }}
NEW_REQUIRED_RESERVE_RATIO: ${{ github.event.inputs.requiredReserveRatio }}
COLLATERAL_TOKEN_ADDRESSES: ${{ github.event.inputs.collateralTokens }}
MIN_COLLATERAL_RATIOS: ${{ github.event.inputs.minCollateralRatios }}
PROFIT_MAX_UNLOCK_TIME: ${{ github.event.inputs.profitMaxUnlock }}
Comment on lines +53 to +84
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add post-deployment verification steps.

The workflow would benefit from additional steps to verify the deployment success:

  1. Verify the deployed contract is operational
  2. Check if the strategy is properly initialized
  3. Validate key parameters

Would you like me to provide an example of post-deployment verification steps?

🧰 Tools
🪛 actionlint (1.7.4)

61-61: shellcheck reported issue in this script: SC2086:info:1:61: Double quote to prevent globbing and word splitting

(shellcheck)

🪛 yamllint (1.35.1)

[warning] 63-63: too many spaces after colon

(colons)


79 changes: 79 additions & 0 deletions .github/workflows/deploy-avax-vault.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
name: "[avalanche-deploy] deploy vault"
on:
workflow_dispatch:
inputs:
asset:
description: 'Asset address'
required: true
default: '0x'
vaultName:
description: 'Yearn vault name'
required: true
default: '0x'
vaultSymbol:
description: 'Yearn vault symbol'
required: true
default: '0x'
profitMaxUnlockTime:
description: 'Profit max unlock time'
required: true
default: '0'
depositLimit:
description: 'Deposit limit'
required: false
default: '0'
defaultPerformance:
description: 'Default performance fee'
required: false
default: '0'
defaultMaxFee:
description: 'Default max fee'
required: false
default: '0'
defaultMaxGain:
description: 'Default max gain'
required: false
default: '0'
defaultMaxLoss:
description: 'Default max loss'
required: false
default: '0'
Comment on lines +5 to +40
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add input validation and improve default values

Several improvements needed for the workflow inputs:

  1. The default value '0x' for addresses is invalid. Consider removing default values for required addresses.
  2. Add input validation for Ethereum addresses using a pattern constraint.
  3. Document the expected units for numerical inputs (e.g., profitMaxUnlockTime in seconds? depositLimit in wei?).

Example fix for the asset input:

   asset:
     description: 'Asset address'
     required: true
-    default: '0x'
+    pattern: '^0x[a-fA-F0-9]{40}$'

Committable suggestion skipped: line range outside the PR's diff.



jobs:
deploy:
runs-on: ubuntu-latest
environment:
name: avalanche
url: https://term-finance.github.io/yearn-v3-term-vault/
steps:
- uses: actions/checkout@master
with:
fetch-depth: 0
submodules: recursive
- uses: foundry-rs/foundry-toolchain@v1
- run: forge install
- run: forge build
- run: forge tree
- run: forge script script/VaultManagement.s.sol:SetupVaultManagement --rpc-url $RPC_URL --broadcast --verify --verbosity 4
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix script command quoting

The forge script command should have quotes around the script path and arguments to prevent issues with special characters.

-      - run: forge script script/VaultManagement.s.sol:SetupVaultManagement --rpc-url $RPC_URL --broadcast --verify --verbosity 4
+      - run: forge script "script/VaultManagement.s.sol:SetupVaultManagement" --rpc-url "$RPC_URL" --broadcast --verify --verbosity 4
📝 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
- run: forge script script/VaultManagement.s.sol:SetupVaultManagement --rpc-url $RPC_URL --broadcast --verify --verbosity 4
- run: forge script "script/VaultManagement.s.sol:SetupVaultManagement" --rpc-url "$RPC_URL" --broadcast --verify --verbosity 4
🧰 Tools
🪛 actionlint (1.7.4)

58-58: shellcheck reported issue in this script: SC2086:info:1:74: Double quote to prevent globbing and word splitting

(shellcheck)

env:
RPC_URL: ${{ secrets.RPC_URL }}
PRIVATE_KEY: ${{ secrets.PRIVATE_KEY }}
ETHERSCAN_API_KEY: ${{ secrets.ETHERSCAN_API_KEY }}
ASSET_ADDRESS: ${{ github.event.inputs.asset }}
VAULT_FACTORY: ${{ vars.VAULT_FACTORY }}
ACCOUNTANT_FACTORY: ${{ vars.ACCOUNTANT_FACTORY }}
VAULT_NAME: ${{ github.event.inputs.vaultName }}
VAULT_SYMBOL: ${{ github.event.inputs.vaultSymbol }}
FEE_RECIPIENT: ${{ vars.FEE_RECIPIENT }}
DEPOSIT_LIMIT: ${{ github.event.inputs.depositLimit }}
VAULT_GOVERNANCE_FACTORY: ${{ vars.VAULT_GOVERNANCE_FACTORY }}
KEEPER_ADDRESS: ${{ vars.KEEPER }}
STRATEGY_ADDER: ${{ vars.STRATEGY_ADDER }}
PROFIT_MAX_UNLOCK_TIME: ${{ github.event.inputs.profitMaxUnlockTime }}
DEFAULT_PERFORMANCE: ${{ github.event.inputs.defaultPerformance }}
DEFAULT_MAX_FEE: ${{ github.event.inputs.defaultMaxFee }}
DEFAULT_MAX_GAIN: ${{ github.event.inputs.defaultMaxGain }}
DEFAULT_MAX_LOSS: ${{ github.event.inputs.defaultMaxLoss }}
Comment on lines +59 to +77
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add environment variable verification and post-deployment checks

  1. Add verification step for required environment variables
  2. Capture and verify the deployed vault address
  3. Add post-deployment verification steps

Add these steps after the deployment:

      - name: Verify environment variables
        run: |
          required_vars=("RPC_URL" "PRIVATE_KEY" "ETHERSCAN_API_KEY" "VAULT_FACTORY" "ACCOUNTANT_FACTORY")
          for var in "${required_vars[@]}"; do
            if [ -z "${!var}" ]; then
              echo "Error: Required environment variable $var is not set"
              exit 1
            fi
          done

      - name: Verify deployment
        run: |
          # Add verification of the deployed vault
          # Example: Check if the vault contract is verified on Snowtrace
          # Check if the vault parameters match the inputs
🧰 Tools
🪛 yamllint (1.35.1)

[warning] 60-60: too many spaces after colon

(colons)



Loading