-
Notifications
You must be signed in to change notification settings - Fork 3
Add tests for deterministic Safe deployment (#37) #38
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile found no issues!From now on, if a review finishes and we haven't found any issues, we will not post anything, but you can confirm that we reviewed your changes in the status check section. This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR". |
- ci.yml: unit tests, typecheck, lint on every PR - integration-safe.yml: Safe deployment tests on main/manual - vitest.config.ts: load .env for integration tests - .gitignore: ignore test-results/
- Test threshold calculation for 1-100 founders - Verify ceil(n/2) formula for 3-5 founders - Verify cap at 5 for 6+ founders - Test error on 0 or negative owners
- Verify predictSafeAddress() matches actual deployed address - Verify different saltNonce produces different addresses - Requires SEPOLIA_TEST_WALLET_KEY env var
- Split monolithic actions test into focused unit tests - Remove complex mock-heavy registration flow test - Delete get-company-events.test.ts (low value, just tests mapping) - Keep: availability, cost, owner action tests
- Explain saltNonce uniqueness per company - Document threshold presets - Clarify Safe Protocol Kit usage
e5f9b0a to
e5e6278
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds comprehensive test coverage and CI workflows for the existing deterministic (CREATE2) Safe deployment functionality. The implementation was already working correctly, and this PR purely adds tests, documentation, and automation without modifying any business logic.
Changes:
- Added 13 unit tests for Safe threshold calculation logic
- Added Sepolia integration tests to verify predicted addresses match deployed addresses
- Added CI workflows for automated testing and quality checks
- Refactored existing test suite for better organization and maintainability
- Added vitest configuration for environment variable loading
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/blockchain/safe-factory.ts | Added comprehensive documentation for CREATE2 deployment and saltNonce usage |
| src/lib/blockchain/safe-factory.test.ts | Added unit tests for calculateThreshold() and getThresholdDescription() functions |
| src/lib/blockchain/safe-factory.integration.test.ts | Added Sepolia integration tests for deterministic Safe deployment |
| src/app/(app)/dashboard/setup/actions.test.ts | Refactored tests into focused describe blocks, removed unnecessary mocks |
| src/lib/blockchain/get-company-events.test.ts | Deleted low-value test file |
| vitest.config.ts | Added configuration for environment variable loading |
| package.json | Added packageManager field for Yarn 4.12.0 |
| .gitignore | Added test-results directory to gitignore |
| .github/workflows/ci.yml | Added CI workflow for unit tests, type checking, and linting |
| .github/workflows/integration-safe.yml | Added Safe deployment integration test workflow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Closes #37
Key finding: The deterministic CREATE2 deployment was already implemented. This PR adds test coverage and documentation to verify and formalize the existing behavior.
What Was Already Done (No Changes Needed)
predictSafeAddress()uses Protocol Kit'spredictedSafeconfig ✅saltNoncegenerated per company:${timestamp}${labelHash}✅What This PR Adds
calculateThreshold()ci.yml(every PR),integration-safe.yml(main/manual)Answers to Issue Questions
0x4e1DCf7AD4e460CfD30791CCC4F9c8a4f820ec67)@safe-global/protocol-kit(current)Test Commands
CI Setup Required
Add
SEPOLIA_TEST_WALLET_KEYsecret in GitHub repo settings (funded Sepolia wallet).Greptile Summary
This PR adds test coverage and documentation for the existing deterministic Safe deployment implementation. The key finding was that CREATE2 deployment was already working correctly via Protocol Kit - this PR validates and formalizes that behavior.
What was added:
calculateThreshold()covering edge cases (solo, 2-founder, multi-founder, cap at 5)ci.ymlruns unit tests/typecheck/lint on every PR;integration-safe.ymlruns Sepolia deployment tests on main pushessafe-factory.tsexplaining saltNonce strategy:${timestamp}${labelHash}generates unique addresses per companyCritical for production:
SEPOLIA_TEST_WALLET_KEYsecret to GitHub repo settings (funded Sepolia wallet needed for integration tests)Confidence Score: 5/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant Dev as Developer participant GH as GitHub PR participant CI as CI Workflow participant Unit as Unit Tests participant Int as Integration Tests participant Sepolia as Sepolia Testnet Dev->>GH: Push PR GH->>CI: Trigger ci.yml CI->>Unit: yarn test:unit Unit->>Unit: Test calculateThreshold() Unit->>Unit: Test getThresholdDescription() Unit-->>CI: ✓ 13 tests pass CI->>CI: yarn tsc --noEmit CI-->>GH: ✓ Typecheck pass CI->>CI: yarn lint CI-->>GH: ✓ Lint pass alt On main branch push or manual trigger GH->>Int: Trigger integration-safe.yml Int->>Int: Load SEPOLIA_TEST_WALLET_KEY Int->>Sepolia: predictSafeAddress(owners, saltNonce) Sepolia-->>Int: 0xPredictedAddress Int->>Sepolia: deploySafe(owners, saltNonce) Sepolia-->>Int: 0xDeployedAddress Int->>Int: Assert predicted === deployed Int-->>GH: ✓ Addresses match (CREATE2 works) end GH-->>Dev: All checks pass ✓