Convert Hardhat tasks to Commander.js commands (using the Call example)#270
Convert Hardhat tasks to Commander.js commands (using the Call example)#270fadeev merged 3 commits intoupdate-clifrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
fadeev
changed the title
|
@hernan-clich lmk what you think of this general direction. |
hernan-clich
left a comment
hernan-clich
left a comment
There was a problem hiding this comment.
The overall direction is fine, this should definitely be the new standard. A comment below on type safety.
|
|
||
| import { createCommand, createRevertOptions, getAbi } from "../common"; | ||
|
|
||
| const main = async (options: any) => { |
There was a problem hiding this comment.
Let's ensure we don't use any, these commands would benefit from zod argument validation, this function is an excellent example of why this is necessary because it uses types and values, and since we're not using any validation or refinements, they could differ in length. We could use a commander preAction hook if we think zod is overkill for this small project.
There was a problem hiding this comment.
Examples is a different sort of code base where we can't have extra code laying around. I agree with you about additional validation and I think we can accomplish this by exporting required validation rules/functions from the toolkit and just importing them here.
There was a problem hiding this comment.
I suppose we can add zod directly here.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Unresolved Merge Conflicts in `yarn.lock`
The yarn.lock file contains unresolved Git merge conflict markers (<<<<<<< HEAD, =======, >>>>>>> update-cli) at multiple locations. This prevents yarn install from completing successfully and causes project build failures.
examples/call/yarn.lock#L135-L283
example-contracts/examples/call/yarn.lock
Lines 135 to 283 in 0d647e4
examples/call/yarn.lock#L5837-L5848
example-contracts/examples/call/yarn.lock
Lines 5837 to 5848 in 0d647e4
Bug: Command Options Type and Requirement Errors
The universal-call and universal-withdraw-and-call commands have two issues:
- The
--functionoption is defined as optional but is directly used inethers.id(). If not provided,ethers.id(undefined)will throw an error, as the function signature is essential for creating the payload. This option should be required. - The
--call-options-gas-limitoption is defined without a type, defaulting to a string. It is then used as agasLimitparameter, which expects a number, potentially causing runtime errors.
examples/call/commands/universal/withdrawAndCall.ts#L56-L61
example-contracts/examples/call/commands/universal/withdrawAndCall.ts
Lines 56 to 61 in 0d647e4
examples/call/commands/universal/call.ts#L14-L15
example-contracts/examples/call/commands/universal/call.ts
Lines 14 to 15 in 0d647e4
examples/call/commands/universal/withdrawAndCall.ts#L18-L19
example-contracts/examples/call/commands/universal/withdrawAndCall.ts
Lines 18 to 19 in 0d647e4
Bug: Function Option Required, Gas Limit Type Mismatch
The --function option is defined as optional but its value is directly used by ethers.id(), causing an error if undefined. It should be a required option. Additionally, the --call-options-gas-limit option is defined as a string but is used as a number for gasLimit, which can cause runtime errors.
examples/call/commands/universal/call.ts#L49-L54
example-contracts/examples/call/commands/universal/call.ts
Lines 49 to 54 in 0d647e4
Comment bugbot run to trigger another review on this PR
Was this report helpful? Give feedback by reacting with 👍 or 👎
2 participants
✅ Transaction sent: 0x7534ce013e1f5fae9972713600a4f7867e1e7a8656092aca2e831c8e73da8e05
✅ Transaction sent: 0xc81efd489f2d8a635ffc84e7cc46cef06de32d3aa826145698ae138d7419ff9c
✅ Transaction sent: 0xbab9478835229a29f02d88b23774ed3123b407f3a8075bdcec1443d988832476
✅ Transaction sent: 0xe9868c5128a9f466eb1898d4ba46232265c1410da8f09c5d14faa90aa8f67c81
✅ Transaction sent: 0xa9e1040a3c60ebc8f8d46671a5d0477f1882984cab73a70fd9e1c8ad2c2ec5e2
Updated to Ethers v6.
Should work with zeta-chain/cli#23
Not implemented yet: