From 0d2c4f79f2c9748d5023049394f5ef4e4bad229c Mon Sep 17 00:00:00 2001 From: amusingaxl <112016538+amusingaxl@users.noreply.github.com> Date: Fri, 9 Feb 2024 10:10:56 -0300 Subject: [PATCH 1/7] refactor: add instructions on how to deal with the `skipped` modifier --- spell/spell-crafter-goerli-workflow.md | 6 ++++-- spell/spell-crafter-mainnet-workflow.md | 6 ++++-- spell/spell-reviewer-goerli-checklist.md | 5 ++++- spell/spell-reviewer-mainnet-checklist.md | 5 ++++- 4 files changed, 16 insertions(+), 6 deletions(-) diff --git a/spell/spell-crafter-goerli-workflow.md b/spell/spell-crafter-goerli-workflow.md index 93bb33e8..2074a831 100644 --- a/spell/spell-crafter-goerli-workflow.md +++ b/spell/spell-crafter-goerli-workflow.md @@ -25,7 +25,7 @@ PR: https://github.com/makerdao/spells-goerli/pull/ * [ ] Disable specific tests IF Not Used (e.g. `testCollateralIntegrations`, `testNewChainlogValues`, `testNewIlkRegistryValues`, ...) * [ ] Remove spell-specific part * [ ] Keep setup - * [ ] Disable by setting visibility to `private` + * [ ] Skip by setting adding the `skipped` modifier * [ ] Add commented notes * [ ] e.g. `// Insert new collateral integration tests here` * [ ] Keep commented tests (e.g. `testOSMs`, `testMedianizers`) @@ -110,7 +110,9 @@ PR: https://github.com/makerdao/spells-goerli/pull/ * [ ] Add new ChainLog Address in `addresses_goerli.sol` (e.g. Collateral Onboarding) * [ ] Run Tests `make test` or `make test match=` to inspect debug traces * [ ] Ensure Good Coverage - * [ ] Ensure every test function is declared as public if enabled or private if disabled + * [ ] Ensure every test function is declared as `public` + * [ ] IF the test is enabled, it MUST NOT have the `skipped` modifier + * [ ] OTHERWISE, if the test is disabled, it MUST have the `skipped` modifier * [ ] Tests PASS via `make test` * [ ] Open PR & Add Reviewers * [ ] Iterate until polls are ended and exec doc is confirmed diff --git a/spell/spell-crafter-mainnet-workflow.md b/spell/spell-crafter-mainnet-workflow.md index 32dac88d..c7addcda 100644 --- a/spell/spell-crafter-mainnet-workflow.md +++ b/spell/spell-crafter-mainnet-workflow.md @@ -26,7 +26,7 @@ Repo: https://github.com/makerdao/spells-mainnet * [ ] Disable specific tests IF Not Used (e.g. `testCollateralIntegrations`, `testNewChainlogValues`, `testNewIlkRegistryValues`, ...) * [ ] Remove spell-specific part * [ ] Keep setup - * [ ] Disable by setting visibility to `private` + * [ ] Skip by setting adding the `skipped` modifier * [ ] Add commented notes * [ ] e.g. `// Insert new collateral integration tests here` * [ ] Keep commented tests (e.g. `testOSMs`, `testMedianizers`) @@ -117,7 +117,9 @@ Repo: https://github.com/makerdao/spells-mainnet * [ ] Add new ChainLog Address in `addresses_mainnet.sol` (e.g. Collateral Onboarding) * [ ] Run Tests `make test` or `make test match=` to inspect debug traces * [ ] Ensure Good Coverage - * [ ] Ensure every test function is declared as public if enabled or private if disabled + * [ ] Ensure every test function is declared as `public` + * [ ] IF the test is enabled, it MUST NOT have the `skipped` modifier + * [ ] OTHERWISE, if the test is disabled, it MUST have the `skipped` modifier * [ ] Tests PASS via `make test` * [ ] Open PR & Add Reviewers * [ ] Iterate until polls are ended and exec doc is confirmed diff --git a/spell/spell-reviewer-goerli-checklist.md b/spell/spell-reviewer-goerli-checklist.md index 84edf7d3..aa7caceb 100644 --- a/spell/spell-reviewer-goerli-checklist.md +++ b/spell/spell-reviewer-goerli-checklist.md @@ -243,7 +243,10 @@ Spell Actions (Per Exec Sheet): * [ ] Tests * [ ] Ensure each spell action has sufficient test coverage _List actions for which coverage was checked here_ - * [ ] Ensure every test function is declared as public if enabled or private if disabled + * [ ] Ensure every test function is declared as `public` + * [ ] IF the test is enabled, it MUST NOT have the `skipped` modifier + * [ ] OTHERWISE, if the test is disabled, it MUST have the `skipped` modifier + * [ ] Check if every test with the `skipped` modifier should be skipped * [ ] Ensure that the `DssExecLib.address` file is not being modified by the spell PR * [ ] Check all CI tests are passing as at the latest commit _Insert most recent commit hash where CI was passing_ diff --git a/spell/spell-reviewer-mainnet-checklist.md b/spell/spell-reviewer-mainnet-checklist.md index 99b82604..f1386241 100644 --- a/spell/spell-reviewer-mainnet-checklist.md +++ b/spell/spell-reviewer-mainnet-checklist.md @@ -313,7 +313,10 @@ * Tests * [ ] Ensure each spell action has sufficient test coverage _List actions for which coverage was checked here_ - * [ ] Ensure every test function is declared as public if enabled or private if disabled + * [ ] Ensure every test function is declared as `public` + * [ ] IF the test is enabled, it MUST NOT have the `skipped` modifier + * [ ] OTHERWISE, if the test is disabled, it MUST have the `skipped` modifier + * [ ] Check if every test with the `skipped` modifier should be skipped * [ ] Ensure that the `DssExecLib.address` file is not being modified by the spell PR * [ ] Check all CI tests are passing as at the latest commit _Insert most recent commit hash where CI was passing_ From 2989365ce4197665af9cb7b43717d966169642c7 Mon Sep 17 00:00:00 2001 From: amusingaxl <112016538+amusingaxl@users.noreply.github.com> Date: Mon, 12 Feb 2024 15:05:33 +0200 Subject: [PATCH 2/7] fix: typos flagged by code review Co-authored-by: SidestreamColdMelon <132689270+SidestreamColdMelon@users.noreply.github.com> --- spell/spell-crafter-goerli-workflow.md | 2 +- spell/spell-crafter-mainnet-workflow.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/spell/spell-crafter-goerli-workflow.md b/spell/spell-crafter-goerli-workflow.md index 2074a831..f2bacd90 100644 --- a/spell/spell-crafter-goerli-workflow.md +++ b/spell/spell-crafter-goerli-workflow.md @@ -25,7 +25,7 @@ PR: https://github.com/makerdao/spells-goerli/pull/ * [ ] Disable specific tests IF Not Used (e.g. `testCollateralIntegrations`, `testNewChainlogValues`, `testNewIlkRegistryValues`, ...) * [ ] Remove spell-specific part * [ ] Keep setup - * [ ] Skip by setting adding the `skipped` modifier + * [ ] Skip by adding the `skipped` modifier * [ ] Add commented notes * [ ] e.g. `// Insert new collateral integration tests here` * [ ] Keep commented tests (e.g. `testOSMs`, `testMedianizers`) diff --git a/spell/spell-crafter-mainnet-workflow.md b/spell/spell-crafter-mainnet-workflow.md index c7addcda..6690ff00 100644 --- a/spell/spell-crafter-mainnet-workflow.md +++ b/spell/spell-crafter-mainnet-workflow.md @@ -26,7 +26,7 @@ Repo: https://github.com/makerdao/spells-mainnet * [ ] Disable specific tests IF Not Used (e.g. `testCollateralIntegrations`, `testNewChainlogValues`, `testNewIlkRegistryValues`, ...) * [ ] Remove spell-specific part * [ ] Keep setup - * [ ] Skip by setting adding the `skipped` modifier + * [ ] Skip by adding the `skipped` modifier * [ ] Add commented notes * [ ] e.g. `// Insert new collateral integration tests here` * [ ] Keep commented tests (e.g. `testOSMs`, `testMedianizers`) From 5940684c0f88dbf4da5cceccb478ee832a734186 Mon Sep 17 00:00:00 2001 From: amusingaxl <112016538+amusingaxl@users.noreply.github.com> Date: Mon, 12 Feb 2024 10:43:06 -0300 Subject: [PATCH 3/7] refactor: improve items related to skipped tests --- spell/spell-reviewer-goerli-checklist.md | 20 ++++++++++---------- spell/spell-reviewer-mainnet-checklist.md | 16 ++++++++-------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/spell/spell-reviewer-goerli-checklist.md b/spell/spell-reviewer-goerli-checklist.md index aa7caceb..3d4d27a2 100644 --- a/spell/spell-reviewer-goerli-checklist.md +++ b/spell/spell-reviewer-goerli-checklist.md @@ -240,18 +240,18 @@ Spell Actions (Per Exec Sheet): * [ ] Ensure every spell variable is declared as `public`/`internal` * [ ] Ensure every spell variable is used in the spell (no unused variables) * [ ] Spell actions match the corresponding [Exec Sheet](https://docs.google.com/spreadsheets/d/1w_z5WpqxzwreCcaveB2Ye1PP5B8QAHDglzyxKHG3CHw) -* [ ] Tests - * [ ] Ensure each spell action has sufficient test coverage - _List actions for which coverage was checked here_ - * [ ] Ensure every test function is declared as `public` - * [ ] IF the test is enabled, it MUST NOT have the `skipped` modifier - * [ ] OTHERWISE, if the test is disabled, it MUST have the `skipped` modifier - * [ ] Check if every test with the `skipped` modifier should be skipped +* Tests * [ ] Ensure that the `DssExecLib.address` file is not being modified by the spell PR * [ ] Check all CI tests are passing as at the latest commit _Insert most recent commit hash where CI was passing_ + * [ ] Ensure every test function is declared as `public` + * [ ] IF the test is enabled, it MUST NOT have the `skipped` modifier + * [ ] OTHERWISE, if the test is disabled, it MUST have the `skipped` modifier + * [ ] Ensure each spell action has sufficient test coverage + _List actions for which coverage was checked here_ + * [ ] Ensure that any other env variable does not affect execution of the tests (for example, by inspecting the output of `printenv | grep "FOUNDRY_\|DAPP_"`) * [ ] Check all tests are passing locally using `make test` - * [ ] Ensure that only `ETH_RPC_URL` is being used from env (i.e. no `match`, `block` or similar are active in your env) + * [ ] Ensure every test listed in the _coverage_ item above is present in the logs with the `[PASS]` prefix. ``` _Insert your passing local tests here_ @@ -283,12 +283,12 @@ _Insert your passing local tests here_ * [ ] Archive matches `src` * [ ] `make diff-archive-spell` for current date or or `date="YYYY-MM-DD" make diff-archive-spell` (date as per cast timestamp) * [ ] Ensure date corresponds to target Exec Sheet date -* [ ] Tests +* Tests * [ ] Ensure that the `DssExecLib.address` file is not being modified by the spell PR * [ ] Check all CI tests are passing as at the latest commit _Insert most recent commit hash where CI was passing_ + * [ ] Ensure that only `ETH_RPC_URL` is being used from env (i.e. no `match`, `block` or similar are active in your env) * [ ] Check all tests are passing locally using `make test` - * [ ] Ensure that only `ETH_RPC_URL` is being used from env (i.e. no `match`, `block` or similar are active in your env) ``` _Insert your passing local tests here_ diff --git a/spell/spell-reviewer-mainnet-checklist.md b/spell/spell-reviewer-mainnet-checklist.md index f1386241..857c521c 100644 --- a/spell/spell-reviewer-mainnet-checklist.md +++ b/spell/spell-reviewer-mainnet-checklist.md @@ -311,19 +311,19 @@ * [ ] Where addresses are fetched from the `ChainLog`, the variable name must match the value of the ChainLog key for that address (e.g. `MCD_VAT` rather than `vat`), except where the archive pattern differs from this pattern (e.g. MKR) * [ ] Spell actions match the corresponding Exec Doc * Tests - * [ ] Ensure each spell action has sufficient test coverage - _List actions for which coverage was checked here_ - * [ ] Ensure every test function is declared as `public` - * [ ] IF the test is enabled, it MUST NOT have the `skipped` modifier - * [ ] OTHERWISE, if the test is disabled, it MUST have the `skipped` modifier - * [ ] Check if every test with the `skipped` modifier should be skipped * [ ] Ensure that the `DssExecLib.address` file is not being modified by the spell PR * [ ] Check all CI tests are passing as at the latest commit _Insert most recent commit hash where CI was passing_ + * [ ] Ensure every test function is declared as `public` + * [ ] IF the test is enabled, it MUST NOT have the `skipped` modifier + * [ ] OTHERWISE, if the test is disabled, it MUST have the `skipped` modifier + * [ ] Ensure each spell action has sufficient test coverage + _List actions for which coverage was checked here_ * [ ] Ensure that any other env variable does not affect execution of the tests (for example, by inspecting the output of `printenv | grep "FOUNDRY_\|DAPP_"`) * [ ] Check all tests are passing locally using `make test` + * [ ] Ensure every test listed in the _coverage_ item above is present in the logs and with the `[PASS]` prefix. -```bash +``` _Insert your local test logs here_ ``` @@ -363,7 +363,7 @@ _Insert your local test logs here_ * [ ] Ensure that any other env variable does not affect execution of the tests (for example, by inspecting the output of `printenv | grep "FOUNDRY_\|DAPP_"`) * [ ] Check all tests are passing locally using `make test` -```bash +``` _Insert your local test logs here_ ``` From 74f36c2aca7980a6a2963ee827c4bdb85e1910e3 Mon Sep 17 00:00:00 2001 From: amusingaxl <112016538+amusingaxl@users.noreply.github.com> Date: Mon, 12 Feb 2024 18:24:02 +0200 Subject: [PATCH 4/7] Update spell/spell-crafter-goerli-workflow.md Co-authored-by: SidestreamColdMelon <132689270+SidestreamColdMelon@users.noreply.github.com> --- spell/spell-crafter-goerli-workflow.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/spell/spell-crafter-goerli-workflow.md b/spell/spell-crafter-goerli-workflow.md index f2bacd90..503e1cd6 100644 --- a/spell/spell-crafter-goerli-workflow.md +++ b/spell/spell-crafter-goerli-workflow.md @@ -111,8 +111,7 @@ PR: https://github.com/makerdao/spells-goerli/pull/ * [ ] Run Tests `make test` or `make test match=` to inspect debug traces * [ ] Ensure Good Coverage * [ ] Ensure every test function is declared as `public` - * [ ] IF the test is enabled, it MUST NOT have the `skipped` modifier - * [ ] OTHERWISE, if the test is disabled, it MUST have the `skipped` modifier + * [ ] IF the test needs to run, it MUST NOT have the `skipped` modifier; OTHERWISE, it MUST have `skipped` modifier * [ ] Tests PASS via `make test` * [ ] Open PR & Add Reviewers * [ ] Iterate until polls are ended and exec doc is confirmed From a1a171d027d74cd308f8023c8c1b198f7b77b6e0 Mon Sep 17 00:00:00 2001 From: amusingaxl <112016538+amusingaxl@users.noreply.github.com> Date: Mon, 12 Feb 2024 15:42:33 -0300 Subject: [PATCH 5/7] fix: update items missed in previous PR --- spell/spell-crafter-mainnet-workflow.md | 3 +-- spell/spell-reviewer-goerli-checklist.md | 3 +-- spell/spell-reviewer-mainnet-checklist.md | 3 +-- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/spell/spell-crafter-mainnet-workflow.md b/spell/spell-crafter-mainnet-workflow.md index 6690ff00..0a286b25 100644 --- a/spell/spell-crafter-mainnet-workflow.md +++ b/spell/spell-crafter-mainnet-workflow.md @@ -118,8 +118,7 @@ Repo: https://github.com/makerdao/spells-mainnet * [ ] Run Tests `make test` or `make test match=` to inspect debug traces * [ ] Ensure Good Coverage * [ ] Ensure every test function is declared as `public` - * [ ] IF the test is enabled, it MUST NOT have the `skipped` modifier - * [ ] OTHERWISE, if the test is disabled, it MUST have the `skipped` modifier + * [ ] IF the test needs to run, it MUST NOT have the `skipped` modifier; OTHERWISE, it MUST have the `skipped` modifier * [ ] Tests PASS via `make test` * [ ] Open PR & Add Reviewers * [ ] Iterate until polls are ended and exec doc is confirmed diff --git a/spell/spell-reviewer-goerli-checklist.md b/spell/spell-reviewer-goerli-checklist.md index 3d4d27a2..3f1e190c 100644 --- a/spell/spell-reviewer-goerli-checklist.md +++ b/spell/spell-reviewer-goerli-checklist.md @@ -245,8 +245,7 @@ Spell Actions (Per Exec Sheet): * [ ] Check all CI tests are passing as at the latest commit _Insert most recent commit hash where CI was passing_ * [ ] Ensure every test function is declared as `public` - * [ ] IF the test is enabled, it MUST NOT have the `skipped` modifier - * [ ] OTHERWISE, if the test is disabled, it MUST have the `skipped` modifier + * [ ] IF the test needs to run, it MUST NOT have the `skipped` modifier; OTHERWISE, it MUST have the `skipped` modifier * [ ] Ensure each spell action has sufficient test coverage _List actions for which coverage was checked here_ * [ ] Ensure that any other env variable does not affect execution of the tests (for example, by inspecting the output of `printenv | grep "FOUNDRY_\|DAPP_"`) diff --git a/spell/spell-reviewer-mainnet-checklist.md b/spell/spell-reviewer-mainnet-checklist.md index 857c521c..201120ed 100644 --- a/spell/spell-reviewer-mainnet-checklist.md +++ b/spell/spell-reviewer-mainnet-checklist.md @@ -315,8 +315,7 @@ * [ ] Check all CI tests are passing as at the latest commit _Insert most recent commit hash where CI was passing_ * [ ] Ensure every test function is declared as `public` - * [ ] IF the test is enabled, it MUST NOT have the `skipped` modifier - * [ ] OTHERWISE, if the test is disabled, it MUST have the `skipped` modifier + * [ ] IF the test needs to run, it MUST NOT have the `skipped` modifier; OTHERWISE, it MUST have the `skipped` modifier * [ ] Ensure each spell action has sufficient test coverage _List actions for which coverage was checked here_ * [ ] Ensure that any other env variable does not affect execution of the tests (for example, by inspecting the output of `printenv | grep "FOUNDRY_\|DAPP_"`) From dd3b9f27d0364bf5d18a9baa3231a0726fa4839c Mon Sep 17 00:00:00 2001 From: amusingaxl <112016538+amusingaxl@users.noreply.github.com> Date: Mon, 12 Feb 2024 15:45:06 -0300 Subject: [PATCH 6/7] fix: add missing term for consistency --- spell/spell-crafter-goerli-workflow.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spell/spell-crafter-goerli-workflow.md b/spell/spell-crafter-goerli-workflow.md index 503e1cd6..9a0d74cb 100644 --- a/spell/spell-crafter-goerli-workflow.md +++ b/spell/spell-crafter-goerli-workflow.md @@ -111,7 +111,7 @@ PR: https://github.com/makerdao/spells-goerli/pull/ * [ ] Run Tests `make test` or `make test match=` to inspect debug traces * [ ] Ensure Good Coverage * [ ] Ensure every test function is declared as `public` - * [ ] IF the test needs to run, it MUST NOT have the `skipped` modifier; OTHERWISE, it MUST have `skipped` modifier + * [ ] IF the test needs to run, it MUST NOT have the `skipped` modifier; OTHERWISE, it MUST have the `skipped` modifier * [ ] Tests PASS via `make test` * [ ] Open PR & Add Reviewers * [ ] Iterate until polls are ended and exec doc is confirmed From 45b65c3829e66713e84af2ce89c2209df6e5c4be Mon Sep 17 00:00:00 2001 From: SidestreamColdMelon <132689270+SidestreamColdMelon@users.noreply.github.com> Date: Wed, 14 Feb 2024 14:22:29 +0100 Subject: [PATCH 7/7] Add tenderly-related checks (#26) --- .wordlist.txt | 1 + spell/spell-crafter-mainnet-workflow.md | 5 +++++ spell/spell-reviewer-mainnet-checklist.md | 7 +++++++ 3 files changed, 13 insertions(+) diff --git a/.wordlist.txt b/.wordlist.txt index 18727a2f..03b61e41 100644 --- a/.wordlist.txt +++ b/.wordlist.txt @@ -125,6 +125,7 @@ Solmate setIlkAutoLineDebtCeiling ESM testnet +testnets authing hardcoded fileables diff --git a/spell/spell-crafter-mainnet-workflow.md b/spell/spell-crafter-mainnet-workflow.md index 0a286b25..241ab377 100644 --- a/spell/spell-crafter-mainnet-workflow.md +++ b/spell/spell-crafter-mainnet-workflow.md @@ -166,9 +166,14 @@ Repo: https://github.com/makerdao/spells-mainnet * [ ] Ensure spell is verified on etherscan * [ ] Ensure local tests PASS against deployed spell run via the deploy script * [ ] Push auto-generated commit +* Cast spell on a newly created Tenderly Testnet + * [ ] Create testnet and cast deployed spell there using `make cast-on-tenderly spell=0x...` command (or a CI trigger) + * [ ] Check that returned `public explorer url` is publicly accessible (e.g. using incognito browser mode) + * [ ] If `cast-on-tenderly` command is executed several times for the same spell, delete all testnets of the same name except the last one * [ ] Archive Spell via `make archive-spell` for current date or `date="YYYY-MM-DD" make archive-spell` (date as per target exec date) * [ ] Commit & Push for Review * [ ] Wait for CI to PASS +* [ ] Post a comment containing links to the deployed spell and Tenderly Testnet * [ ] Wait for at Least two Approvals to Share for Publishing to Governance Facilitators * [ ] Share Deployed Address in [`new-spells`](https://discord.com/channels/893112320329396265/897483518316265553) discord channel * [ ] Make sure to tag responsible governance facilitator in the message with the address diff --git a/spell/spell-reviewer-mainnet-checklist.md b/spell/spell-reviewer-mainnet-checklist.md index 201120ed..24db3b10 100644 --- a/spell/spell-reviewer-mainnet-checklist.md +++ b/spell/spell-reviewer-mainnet-checklist.md @@ -352,6 +352,13 @@ _Insert your local test logs here_ * [ ] Check again that the PR did not modify the `DssExecLib.address` file (e.g. look under the 'Files Changed' PR tab, etc.) * [ ] Ensure Etherscan `Libraries Used` matches DssExecLib [Latest Release](https://github.com/makerdao/dss-exec-lib/releases/latest) * [ ] (For your tests to be accurate) git submodule hash matches [dss-exec-lib](https://github.com/makerdao/dss-exec-lib/releases/latest) latest release's tag commit and inspect diffs if doesn't match to ensure expected behaviour (Currently Non-Critical pending the next DssExecLib release, double check that the ExecLib used by the contract matches the latest release) +* Tenderly Testnet checks + * [ ] A testnet with the name matching spell description is found at [maker dashboard](https://dashboard.tenderly.co/maker/virtual-networks) + * [ ] The testnet name is unique (previous testnets does not have the same name) + * [ ] Cast transaction is set to the correct "receiver" (matches deployed spell address) + * [ ] All actions are executed in the transaction trace + * [ ] No reverts are present that block execution + * [ ] No out-of-gas errors are present * Archive checks * [ ] `make diff-archive-spell` for current date or `make diff-archive-spell date="YYYY-MM-DD"` * [ ] Ensure date corresponds to target Exec Doc date