chore(evm-solidity): Move unrelated docs, gen-embeds, and add Solidity docs #2168
Conversation
won't be published on npm for @nibiruchain/solidity
|
Warning Rate limit exceeded@Unique-Divine has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 17 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
WalkthroughThis pull request introduces a new Changes
Possibly related PRs
Poem
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 (
|
Unique-Divine
changed the title
![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 (3)
x/evm/embeds/HACKING.md (3)
9-20: Enhance build instructions with prerequisites and details.Consider adding:
- Prerequisites for the
justcommand installation- Description of what
gen-embedsdoes- Clear order of operations (what to run first)
## Building Outputs -Workhorse command +### Prerequisites + +1. Install `just` command runner: + ```bash + # macOS + brew install just + + # Linux + curl --proto '=https' --tlsv1.2 -sSf https://just.systems/install.sh | bash + ``` + +### Steps + +1. First, install dependencies and compile contracts: + ```bash + cd x/evm/embeds + npm install + npx hardhat compile + ``` + +2. Generate embeds (updates Go files with compiled contract artifacts): ```bash just gen-embeds
-From inside the "Nibiru/x/evm/embeds" directory
-bash -npm install -npx hardhat compile ---- `22-75`: **Consider adding guidelines for error documentation and validation tools.** The documentation guidelines are comprehensive. Consider enhancing them with: 1. Guidelines for documenting error conditions using `@custom:error` or `@revert` 2. Mention of tools like `solhint` or `hardhat-docgen` for validating NatSpec comments ```diff ### NatSpec Fields - `@notice`: Used to explain to end users what the function does. Should be written in plain English and focus on the function's purpose. Best practice: Include for all public and external functions. - `@param`: Describes a function parameter. Should explain what the parameter is used for. Best practice: Include for all function parameters, especially in interfaces. - `@dev`: Provides additional details for developers. Used for implementation details, notes, or warnings for developers. Best practice: Use when there's important information that doesn't fit in `@notice` but is crucial for developers. - `@return`: Describes what a function returns. Best practice: Use for all functions that return values, explaining each return value. +- `@custom:error`: Documents custom error conditions. + Best practice: Use for all custom errors to explain when they occur. +- `@revert`: Documents conditions that cause the function to revert. + Best practice: Document all revert conditions for better error handling. + +### Validation Tools + +Use these tools to ensure documentation quality: +- `solhint` with NatSpec validation rules +- `hardhat-docgen` for generating documentation +- VS Code Solidity extension for real-time validation🧰 Tools
🪛 LanguageTool
[uncategorized] ~34-~34: Loose punctuation mark.
Context: ...mments. ### NatSpec Fields -@notice: Used to explain to end users what the f...(UNLIKELY_OPENING_PUNCTUATION)
[style] ~34-~34: To form a complete sentence, be sure to include a subject.
Context: ...in to end users what the function does. Should be written in plain English and focus o...(MISSING_IT_THERE)
[uncategorized] ~36-~36: Loose punctuation mark.
Context: ...ublic and external functions. -@param: Describes a function parameter. Should ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~38-~38: Loose punctuation mark.
Context: ...ters, especially in interfaces. -@dev: Provides additional details for develop...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~40-~40: Loose punctuation mark.
Context: ...t is crucial for developers. -@return: Describes what a function returns. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~41-~41: Possible missing comma found.
Context: ... a function returns. Best practice: Use for all functions that return values, e...(AI_HYDRA_LEO_MISSING_COMMA)
76-112: Add gas optimization context to state mutability guidelines.The state mutability section is clear and well-exemplified. Consider adding:
- Gas optimization benefits of using
view/purefunctions- Common pitfalls when choosing between different state mutability types
### State Mutability State mutability defines how a function interacts with the blockchain state. Always explicitly declare non-default state mutability keywords for clarity and correctness. + +#### Gas Optimization Considerations + +- `view` and `pure` functions are free when called externally (no gas cost) +- Marking functions as `view` or `pure` when possible helps users save gas +- Be cautious of state reads in loops even in `view` functions 1. `view` : For stateful queries - Reads state, but cannot modify it.🧰 Tools
🪛 LanguageTool
[uncategorized] ~82-~82: Loose punctuation mark.
Context: ...for clarity and correctness. 1.view: For stateful queries - Reads state, ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~90-~90: Loose punctuation mark.
Context: ...ew returns (uint256); ``` 2.pure: For stateless queries - Neither read...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~106-~106: Loose punctuation mark.
Context: ... amount) external; ``` 4.payable: State mutating operation that can recei...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
x/evm/embeds/HACKING.md(1 hunks)x/evm/embeds/README.md(1 hunks)x/evm/embeds/abi/IFunToken.json(3 hunks)x/evm/embeds/artifacts/contracts/ERC20Minter.sol/ERC20Minter.json(1 hunks)x/evm/embeds/artifacts/contracts/IFunToken.sol/IFunToken.json(3 hunks)x/evm/embeds/artifacts/contracts/TestERC20TransferThenPrecompileSend.sol/TestERC20TransferThenPrecompileSend.json(1 hunks)x/evm/embeds/artifacts/contracts/TestFunTokenPrecompileLocalGas.sol/TestFunTokenPrecompileLocalGas.json(1 hunks)x/evm/embeds/artifacts/contracts/TestInfiniteRecursionERC20.sol/TestInfiniteRecursionERC20.json(1 hunks)x/evm/embeds/artifacts/contracts/TestNativeSendThenPrecompileSend.sol/TestNativeSendThenPrecompileSend.json(1 hunks)x/evm/embeds/artifacts/contracts/TestPrecompileSelfCallRevert.sol/TestPrecompileSelfCallRevert.json(1 hunks)x/evm/embeds/contracts/ERC20Minter.sol(1 hunks)x/evm/embeds/contracts/IFunToken.sol(2 hunks)
✅ Files skipped from review due to trivial changes (7)
- x/evm/embeds/contracts/ERC20Minter.sol
- x/evm/embeds/artifacts/contracts/TestERC20TransferThenPrecompileSend.sol/TestERC20TransferThenPrecompileSend.json
- x/evm/embeds/artifacts/contracts/TestNativeSendThenPrecompileSend.sol/TestNativeSendThenPrecompileSend.json
- x/evm/embeds/artifacts/contracts/TestPrecompileSelfCallRevert.sol/TestPrecompileSelfCallRevert.json
- x/evm/embeds/README.md
- x/evm/embeds/artifacts/contracts/TestFunTokenPrecompileLocalGas.sol/TestFunTokenPrecompileLocalGas.json
- x/evm/embeds/artifacts/contracts/ERC20Minter.sol/ERC20Minter.json
🧰 Additional context used
🪛 LanguageTool
x/evm/embeds/HACKING.md
[uncategorized] ~34-~34: Loose punctuation mark.
Context: ...mments. ### NatSpec Fields - @notice: Used to explain to end users what the f...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~34-~34: To form a complete sentence, be sure to include a subject.
Context: ...in to end users what the function does. Should be written in plain English and focus o...
(MISSING_IT_THERE)
[uncategorized] ~36-~36: Loose punctuation mark.
Context: ...ublic and external functions. - @param: Describes a function parameter. Should ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~38-~38: Loose punctuation mark.
Context: ...ters, especially in interfaces. - @dev: Provides additional details for develop...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~40-~40: Loose punctuation mark.
Context: ...t is crucial for developers. - @return: Describes what a function returns. ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~41-~41: Possible missing comma found.
Context: ... a function returns. Best practice: Use for all functions that return values, e...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~82-~82: Loose punctuation mark.
Context: ...for clarity and correctness. 1. view : For stateful queries - Reads state, ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~90-~90: Loose punctuation mark.
Context: ...ew returns (uint256); ``` 2. pure : For stateless queries - Neither read...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~106-~106: Loose punctuation mark.
Context: ... amount) external; ``` 4. payable : State mutating operation that can recei...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e-evm
🔇 Additional comments (13)
x/evm/embeds/contracts/IFunToken.sol (7)
9-12: LGTM! Clear and concise interface documentation.The interface documentation clearly describes its purpose and functionality.
Line range hint
13-23: LGTM! Well-documented function with clear parameter descriptions.The
sendToBankfunction documentation is comprehensive, including parameter descriptions and return value explanation.
35-47: LGTM! Function signature updated with view modifier.The
balancefunction is correctly marked asviewsince it only reads state. The return types are well-defined.
50-58: LGTM! Function signature updated with view modifier.The
bankBalancefunction is correctly marked asviewsince it only reads state. The return types are well-defined.
60-67: LGTM! Function signature updated with view modifier.The
whoAmIfunction is correctly marked asviewsince it only reads state. The documentation and parameter descriptions are clear.
69-84: LGTM! Comprehensive function documentation.The
sendToEvmfunction documentation is thorough, including detailed parameter descriptions and explaining the underlying logic.
Line range hint
87-98: LGTM! Clear function documentation.The
bankMsgSendfunction documentation clearly describes its purpose and parameters.x/evm/embeds/abi/IFunToken.json (3)
81-81: LGTM! Correct stateMutability update.The
balancefunction's stateMutability is correctly updated to "view" to match the interface.
122-122: LGTM! Correct stateMutability update.The
bankBalancefunction's stateMutability is correctly updated to "view" to match the interface.
240-240: LGTM! Correct stateMutability update.The
whoAmIfunction's stateMutability is correctly updated to "view" to match the interface.x/evm/embeds/artifacts/contracts/TestInfiniteRecursionERC20.sol/TestInfiniteRecursionERC20.json (1)
312-313: LGTM! Updated contract bytecode.The bytecode has been updated but maintains the same functionality as indicated by the unchanged ABI.
x/evm/embeds/artifacts/contracts/IFunToken.sol/IFunToken.json (1)
85-85: Appropriate state mutability updates for read-only functions.The changes from
"nonpayable"to"view"forbalance,bankBalance, andwhoAmIfunctions are correct since these functions only read state without modifying it. This change:
- More accurately reflects the functions' behavior
- Enables gas optimizations
- Allows these functions to be called from other
viewfunctionsLet's verify that these functions don't modify state by checking their implementations:
Also applies to: 126-126, 244-244
✅ Verification successful
Verified: State mutability changes are correct
The interface implementation confirms that
balance,bankBalance, andwhoAmIfunctions are properly marked asviewsince they only return data without modifying state:
balance: Returns balances and token metadatabankBalance: Returns bank balance and account infowhoAmI: Returns address resolution data🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that these functions don't modify state # Expected: Function bodies should only contain read operations ast-grep --pattern 'function $_($$) { $$$ $write $$$ }' -l solidityLength of output: 192
Script:
#!/bin/bash # Find and display the Solidity interface file fd -e sol "IFunToken" --exec cat {}Length of output: 3925
x/evm/embeds/HACKING.md (1)
1-8: Well-structured documentation with clear navigation!The table of contents is well-organized and provides easy navigation to all major sections.
Purpose / Abstract
Summary by CodeRabbit
Documentation
HACKING.mdguide for Solidity development best practices.README.mdto reference the new hacking guide.Code Improvements
IFunTokencontract toview.Solidity Conventions