feat(Escrow): foundry test #104
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces new submodules for external libraries and updates to configuration and remapping files. It enhances event logging in the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant ES as EscrowUniversal
participant AM as ArbitratorMock
U->>ES: Initiate transaction/dispute process
ES->>AM: Forward dispute details
AM-->>ES: Return ruling decision
ES->>U: Emit TransactionResolved event
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for kleros-escrow-v2 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
contracts/test/EscrowUniversal.t.sol (2)
52-55: Consider differentiating event names to avoid confusion.
Currently, there are two "Ruling" events with different parameter types, which may cause confusion when filtering logs.
280-620: Potential to reduce repetitive code.
Many test functions follow a similar pattern of creating a transaction, proposing a settlement, paying fees, etc. Consider extracting common setup and execution steps into helper functions to streamline the tests and improve readability.contracts/src/test/TestERC20.sol (1)
9-9: Allow configurable token supply for broader testing scenarios.
Currently, the initial mint amount is hard-coded to 1,000,000 tokens. Allowing a constructor parameter or a func to mint additional tokens can provide extra flexibility for certain tests.contracts/src/test/DisputeTemplateRegistryMock.sol (1)
16-17: Consider storing additional template data.
Since this is a mock, it’s understandable to keep it simple. However, if you need to test retrieval or advanced template logic later, consider storing and exposing the registered template details in a mapping.contracts/src/test/ArbitratorMock.sol (1)
27-27: Consider using a mapping for disputes storageUsing an array for disputes storage could lead to high gas costs when accessing disputes with high IDs. Consider using a mapping instead.
- Dispute[] public disputes; + mapping(uint256 => Dispute) public disputes; + uint256 public disputesCount;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.gitmodules(1 hunks)contracts/foundry.toml(1 hunks)contracts/lib/forge-std(1 hunks)contracts/lib/solmate(1 hunks)contracts/remappings.txt(1 hunks)contracts/src/EscrowUniversal.sol(1 hunks)contracts/src/test/ArbitratorMock.sol(1 hunks)contracts/src/test/DisputeTemplateRegistryMock.sol(1 hunks)contracts/src/test/TestERC20.sol(1 hunks)contracts/test/EscrowUniversal.t.sol(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- contracts/foundry.toml
- contracts/lib/forge-std
- contracts/lib/solmate
- .gitmodules
🧰 Additional context used
🪛 LanguageTool
contracts/remappings.txt
[misspelling] ~1-~1: Hoewel het woord correct kan zijn, is er een grote kans dat het een typefout is van “noden”. Zo niet, dan is het waarschijnlijk een ouderwetse uitdrukking.
Context: @openzeppelin/=../node_modules/@openzeppelin/ @kleros/=../node...
(MACHTE)
[misspelling] ~2-~2: Hoewel het woord correct kan zijn, is er een grote kans dat het een typefout is van “noden”. Zo niet, dan is het waarschijnlijk een ouderwetse uitdrukking.
Context: ...node_modules/@openzeppelin/ @kleros/=../node_modules/@kleros/ ds-test/=lib/forge-std...
(MACHTE)
[misspelling] ~4-~4: Hoewel het woord correct kan zijn, is er een grote kans dat het een typefout is van “noden”. Zo niet, dan is het waarschijnlijk een ouderwetse uitdrukking.
Context: ...d/lib/ds-test/src/ eth-gas-reporter/=../node_modules/eth-gas-reporter/ forge-std/=li...
(MACHTE)
[misspelling] ~6-~6: Hoewel het woord correct kan zijn, is er een grote kans dat het een typefout is van “noden”. Zo niet, dan is het waarschijnlijk een ouderwetse uitdrukking.
Context: .../=lib/forge-std/src/ hardhat-deploy/=../node_modules/hardhat-deploy/ hardhat/=../nod...
(MACHTE)
[misspelling] ~7-~7: Hoewel het woord correct kan zijn, is er een grote kans dat het een typefout is van “noden”. Zo niet, dan is het waarschijnlijk een ouderwetse uitdrukking.
Context: ...ode_modules/hardhat-deploy/ hardhat/=../node_modules/hardhat/ solmate/=lib/solmate/s...
(MACHTE)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - kleros-escrow-v2
- GitHub Check: Header rules - kleros-escrow-v2
- GitHub Check: Pages changed - kleros-escrow-v2
🔇 Additional comments (4)
contracts/test/EscrowUniversal.t.sol (1)
1-1420: Comprehensive test coverage.
The test suite thoroughly verifies various scenarios, including boundary and revert conditions, ensuring strong coverage of escrow functionality.contracts/src/test/ArbitratorMock.sol (1)
95-95: Consider handling the return value of .send()The .send() call could fail silently. Consider using a safer transfer method or at least checking the return value.
- payable(msg.sender).send(dispute.fees); // Avoid blocking. + bool success = payable(msg.sender).send(dispute.fees); + require(success, "Fee transfer failed");contracts/src/EscrowUniversal.sol (1)
430-430: LGTM! Event emission moved to a more appropriate locationMoving the TransactionResolved event to the rule function is a good change as it provides better tracking of dispute resolution flow. This change ensures the event is emitted once per resolution, regardless of whether it's triggered by timeOutByBuyer, timeOutBySeller, or executeRuling.
contracts/remappings.txt (1)
1-8: LGTM! Standard remappings for project dependenciesThe remappings follow standard conventions and include all necessary dependencies for the project.
🧰 Tools
🪛 LanguageTool
[misspelling] ~1-~1: Hoewel het woord correct kan zijn, is er een grote kans dat het een typefout is van “noden”. Zo niet, dan is het waarschijnlijk een ouderwetse uitdrukking.
Context: @openzeppelin/=../node_modules/@openzeppelin/ @kleros/=../node...(MACHTE)
[misspelling] ~2-~2: Hoewel het woord correct kan zijn, is er een grote kans dat het een typefout is van “noden”. Zo niet, dan is het waarschijnlijk een ouderwetse uitdrukking.
Context: ...node_modules/@openzeppelin/ @kleros/=../node_modules/@kleros/ ds-test/=lib/forge-std...(MACHTE)
[misspelling] ~4-~4: Hoewel het woord correct kan zijn, is er een grote kans dat het een typefout is van “noden”. Zo niet, dan is het waarschijnlijk een ouderwetse uitdrukking.
Context: ...d/lib/ds-test/src/ eth-gas-reporter/=../node_modules/eth-gas-reporter/ forge-std/=li...(MACHTE)
[misspelling] ~6-~6: Hoewel het woord correct kan zijn, is er een grote kans dat het een typefout is van “noden”. Zo niet, dan is het waarschijnlijk een ouderwetse uitdrukking.
Context: .../=lib/forge-std/src/ hardhat-deploy/=../node_modules/hardhat-deploy/ hardhat/=../nod...(MACHTE)
[misspelling] ~7-~7: Hoewel het woord correct kan zijn, is er een grote kans dat het een typefout is van “noden”. Zo niet, dan is het waarschijnlijk een ouderwetse uitdrukking.
Context: ...ode_modules/hardhat-deploy/ hardhat/=../node_modules/hardhat/ solmate/=lib/solmate/s...(MACHTE)
One small contract change:
TransactionResolvedevent moved fromexecuteRulingtoruleinstead since the former can also be triggered bytimeOutByBuyerandtimeOutBySellerwhich emit the same eventPR-Codex overview
This PR focuses on adding new contracts and tests for an escrow system, enhancing dispute resolution, and integrating ERC20 token handling with arbitration functionality.
Detailed summary
TestERC20contract for testing ERC20 functionalities.DisputeTemplateRegistryMockandArbitratorMockfor dispute handling.EscrowUniversalto emit appropriate events for transaction resolutions.EscrowUniversalTestto cover various scenarios, including arbitration fees and settlement proposals.remappings.txt.Summary by CodeRabbit
New Features
Chores