feat: add faucet contract #110
Conversation
WalkthroughThis pull request introduces three new Solidity smart contracts: Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (3)
script/18_SimulateReceive.s.sol (2)
50-54: Clarify the use of theencodedvariableThe variable
encodedis created usingabi.encodeWithSelectorbut is only used in a debugconsole.logBytesstatement.If the
encodeddata is for debugging purposes, consider adding a comment to explain its purpose. If it's no longer needed, you might remove it to clean up the code.bytes memory encoded = abi.encodeWithSelector( IOAppCore(receiver).endpoint().lzReceive.selector, origin, receiver, guid, payload, extraData ); console.logBytes(encoded); +// Encoded data for debugging purposesOr, if unnecessary:
-bytes memory encoded = abi.encodeWithSelector( - IOAppCore(receiver).endpoint().lzReceive.selector, origin, receiver, guid, payload, extraData -); -console.logBytes(encoded);
57-59: Simplify theaddressToBytes32functionThe current implementation correctly converts an
addresstobytes32, but it can be simplified for readability.Consider using built-in Solidity functions for clarity:
function addressToBytes32(address _addr) internal pure returns (bytes32) { - return bytes32(uint256(uint160(_addr))); + return bytes32(uint256(uint160(address(_addr)))); }Or, since an address occupies the lower 20 bytes of a
bytes32, you can shift the address to the correct position:function addressToBytes32(address _addr) internal pure returns (bytes32) { return bytes32(uint256(uint160(_addr)) << 96); }This makes the intention of the code clearer to readers.
test/foundry/unit/CombinedFaucet.t.sol (1)
291-296: Relevance of ERC721 rejection test inNativeTokenFaucetTestWhile it's useful to test that the faucet rejects ERC721 tokens, consider whether this test is necessary in the context of a native token faucet. If the
onERC721Receivedfunction is not applicable, it might be more appropriate to test for incorrect calls or misuse relevant to native tokens.Evaluate if this test should be adjusted or potentially removed to better align with the faucet's functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- script/18_SimulateReceive.s.sol (1 hunks)
- script/19_DeployFaucet.s.sol (1 hunks)
- src/core/CombinedFaucet.sol (1 hunks)
- test/foundry/unit/CombinedFaucet.t.sol (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
test/foundry/unit/CombinedFaucet.t.sol (3)
90-100: Test ownership restrictions more thoroughlyIn
testOnlyOwnerCanSetTokenAddress, after setting the token address toaddress(0xdead), it's good practice to verify that the faucet still functions correctly or behaves as expected with the new token address.You can add assertions or additional test cases to confirm that the faucet's behavior aligns with expectations after the token address is changed.
149-153: EnsureonERC721Receivedfunction is implemented in the contractThe test
testRejectERC721is checking the behavior when receiving ERC721 tokens:faucet.onERC721Received(user1, user2, 1, "");Verify that the
CombinedFaucetcontract properly implements theonERC721Receivedfunction and handles rejections as intended.Check the
CombinedFaucetcontract to ensure theonERC721Receivedfunction is present and reverts with the correct message.
32-34: Ensure correct token distribution in testsIn the
setUpfunction, tokens are minted touser1:token.mint(user1, tokenAmount);Consider whether it's necessary to mint tokens to
user1for these tests, especially since the faucet is supposed to distribute tokens to users who may not have any. This can help ensure that the tests accurately reflect the intended use case.To verify if
user1requires an initial balance, you can temporarily remove the minting line and run the tests to see if they pass without it.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
test/foundry/unit/CombinedFaucet.t.sol (6)
295-295: Double-check initial balance intestRecoverTokensThe comment states:
// Initially, owner has 10 * `tokenAmount` native tokensHowever, since
vm.deal(owner, tokenAmount * 10);is called in thesetUpfunction, it's worth verifying that the owner's balance is indeedtokenAmount * 10before proceeding. Additionally, consider adding an assertion to confirm this initial balance within the test for clarity.You can add the following assertion:
assertEq(owner.balance, tokenAmount * 10);This ensures the test explicitly confirms the initial state.
181-181: Confirm the faucet's balance setupIn the
setUpfunction ofNativeTokenFaucetTest, you dealtokenAmount * 5to the faucet:vm.deal(address(faucet), tokenAmount * 5);Ensure that this setup correctly aligns with your testing scenarios, especially if subsequent tests depend on the faucet's balance. Consider adding comments to clarify the purpose of this step for future maintainability.
308-312: Update revert message intestRejectERC721The revert message in
testRejectERC721is:vm.expectRevert("Faucet: ERC721 tokens not accepted");For consistency with other error messages in the
CombinedFaucetcontract, consider updating the message to include "CombinedFaucet" as a prefix.Apply this diff:
-vm.expectRevert("Faucet: ERC721 tokens not accepted"); +vm.expectRevert("CombinedFaucet: ERC721 tokens not accepted");Ensure that the
CombinedFaucetcontract's revert message is updated accordingly.
154-155: Clarify the recovered token amount in the commentThe comment states:
// Owner should recover `tokenAmount` TST tokensFor clarity, specify the exact amount to make it explicit in the comment, enhancing readability.
Update the comment:
-// Owner should recover `tokenAmount` TST tokens +// Owner should recover 1 TST token (`tokenAmount`)
46-46: Remove redundant commentThe comment:
// Deploy a mock ERC20 tokenIs somewhat redundant as the code directly below is self-explanatory. Consider removing or enhancing the comment to provide additional value.
Optionally, you could modify it to:
// Initialize mock ERC20 token for testing
11-37: Consider documenting theBaseFaucetTestcontractAdding documentation comments to the
BaseFaucetTestcontract can help other developers understand its purpose and how it facilitates the shared setup for both faucet tests.Consider adding a contract-level comment:
/// @title BaseFaucetTest /// @notice Provides common setup and utilities for faucet tests contract BaseFaucetTest is Test { // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- script/19_DeployFaucet.s.sol (1 hunks)
- src/core/CombinedFaucet.sol (1 hunks)
- test/foundry/unit/CombinedFaucet.t.sol (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- script/19_DeployFaucet.s.sol
- src/core/CombinedFaucet.sol
🧰 Additional context used
|
See also https://github.com/ExocoreNetwork/exo-faucet which is the backend deployed on our URL. |
The
CombinedFaucetcontract supports two types of faucetsaddress = 0x0000000000000000000000000000000000000000). This needs a backend service running to provide funds to users who may not have any funds, including funds to pay for gas to claim from the faucet.The former is deployed on Exocore while the latter is deployed on the client chain. The same script was used to deploy both, depending on an environment variable value. However, the addresses aren't yet made public because faucets must be protected with a frontend captcha to avoid Sybil attacks.
Lastly, a script to simulate LZ receive is added for easy testing of messages that may be stuck.
Summary by CodeRabbit
New Features
SimulateReceivecontract to simulate message reception in a LayerZero context.DeployScriptcontract for deploying a faucet contract based on specific conditions.CombinedFaucetcontract for controlled distribution of ERC20 tokens or native Ether.Bug Fixes
SimulateReceivecontract.Tests
CombinedFaucetcontract, covering both ERC20 and native token functionalities.