Add Gringotts scenario test scripts#29
Conversation
PR SummaryMedium Risk Overview Introduces Updates the core contract to revert on removing the last operator ( Reviewed by Cursor Bugbot for commit b85586e. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| "unlockDistributionAddress": "0x7fd42b44F08eC90Aa7C82f9C40235Dc66b86C201", | ||
| "stakingRewardAddress": "0x6199d949c97e818abd967EC9EcA3e89FFbE92C44", | ||
| "maxVotingPeriod": 300, | ||
| "adminVotingThresholdPercentage": 50, |
There was a problem hiding this comment.
Example config replaced with real testnet-specific values
Medium Severity
The deploy-and-create.example.json template file had its obvious placeholder addresses (0x1111..., 0x2222..., etc.) replaced with real testnet addresses, maxVotingPeriod lowered from 3600 to 300, and adminVotingThresholdPercentage lowered from 75 to 50. The deploy script (deploy-and-create.js) references this file by name as its example config. Replacing self-documenting placeholders with real values means someone following the usage instructions could accidentally deploy against these specific addresses and with reduced governance security parameters.
Reviewed by Cursor Bugbot for commit 2ecf9f1. Configure here.
| Compile contracts first so the scripts can load current ABIs, including `GringottsV2Dummy`: | ||
|
|
||
| ```bash | ||
| cd /Users/xiaoyuchen/repos/gringotts/solidity |
There was a problem hiding this comment.
README contains hardcoded local filesystem paths
Low Severity
The README contains developer-local absolute paths (/Users/xiaoyuchen/Downloads/EVM Gringotts Testing.txt on line 3 and /Users/xiaoyuchen/repos/gringotts/solidity on line 12) that are meaningless and broken for any other developer cloning the repo. These look like they were left in from local development.
Reviewed by Cursor Bugbot for commit 2ecf9f1. Configure here.
| "totalAmount": "3000000000000000000", | ||
| "vestingTotal": "3000000000000000000" | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Testnet deployment artifact committed with local path
Low Severity
A specific testnet deployment output file was committed to the repo. It contains a local filesystem path in configFile (/Users/xiaoyuchen/repos/...), deployer addresses, and transaction hashes from a one-off testnet deployment. The deployments/ directory is not in .gitignore, and loadScenarioConfig() in common.js auto-discovers the latest file from this directory, meaning this committed artifact will silently become the default proxy address for anyone running the scenario scripts.
Reviewed by Cursor Bugbot for commit 2ecf9f1. Configure here.
| runner, | ||
| contract, | ||
| actors.admin, | ||
| actors.secondAdmin || actors.nonAdmin, |
There was a problem hiding this comment.
Non-admin fallback voter breaks expiration test logic
Medium Severity
The fallback actors.secondAdmin || actors.nonAdmin passes a non-admin wallet as the voter to expectVoteAfterExpirationBehavior. The voteProposal contract method requires admin permission, so when secondAdmin is unavailable the transaction reverts due to access control — not due to expiration. The test then reports a misleading failure for the wrong reason. The equivalent scenario in access-tests.js correctly guards with requireActor(runner, actors.secondAdmin, "secondAdmin") and returns early if missing.
Reviewed by Cursor Bugbot for commit 2ecf9f1. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 5 total unresolved issues (including 4 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b85586e. Configure here.
| .sort((a, b) => fs.statSync(b).mtimeMs - fs.statSync(a).mtimeMs); | ||
|
|
||
| if (files.length === 0) return {}; | ||
| return JSON.parse(fs.readFileSync(files[0], "utf8")); |
There was a problem hiding this comment.
Wrong deployment file chosen
Medium Severity
latestDeployment picks the newest file by filesystem mtime, not the embedded deploy timestamp in gringotts-*-<ms>.json. After clone or when mtimes tie, sort order is unstable, so scenario scripts may target the wrong proxy and params.
Reviewed by Cursor Bugbot for commit b85586e. Configure here.
1 participant
Describe your changes and provide context
Testing performed to validate your change