Added async precompile #1160
Added async precompile #1160
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive implementation of an asynchronous precompile system for the Warden blockchain. The changes span multiple files across the project, adding support for managing "futures" - a mechanism for handling asynchronous operations. The implementation includes Solidity interfaces, Go bindings, query methods, transaction handling, and test cases, effectively extending the blockchain's capabilities to support more complex, asynchronous computational workflows. Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
These reviewers have expertise in the Warden Protocol's blockchain infrastructure and precompile systems, making them ideal candidates to review this comprehensive async precompile implementation. Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
Documentation and Community
|
|
Hey @backsapc and thank you for opening this pull request! 👋🏼 It looks like you forgot to add a changelog entry for your changes. Make sure to add a changelog entry in the 'CHANGELOG.md' 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: 1
🧹 Nitpick comments (16)
precompiles/async/types.go (3)
13-17: Add more descriptive struct field commentsThe
FuturesInputstruct would benefit from additional comments explaining the meaning and usage of each field, especially forPagination.
20-37: Check pointer vs. value usage inFromResponseThe method modifies the receiver (
r *FuturesResponse) and then returns*r. Consider returning a newly constructedFuturesResponseto keep the function purely functional, avoiding potential side effects on the original receiver.
59-71: Consistent naming in error messagesIn
FutureByIdResponse.FromResponse, the variableresis consistently named, but the error message (return FutureByIdResponse{}, err) could be more descriptive to clarify the context of the failure (e.g., “error mapping future by ID”).precompiles/async/async.go (2)
44-64: Consider explicitcontext.Contextusage
NewPrecompileuses the loaded ABI and various store configs but does not incorporate a parameterized context for potential timeouts or cancellations. If the system might need more advanced context control (for example, if adding concurrency later), consider introducing acontext.Contextparameter.
131-143: Method mismatch panic
IsTransactionpanics if the method is missing. While that is intentional for a typical precompile, confirm that a panic is acceptable from a user perspective if this method is called with an unknown method, as opposed to returning an error.precompiles/async/query.go (3)
48-61: Improve error contextWhen returning an error from
newFutureByIdRequest, consider appending the method name and argument details for easier debugging, especially if logs are shared across multiple query calls.
94-115:newFuturesRequestfield consistencyIn
newFuturesRequest,creatoris converted to a Bech32 address string or left empty ifinput.Creatoris zero. Confirm that an empty string does not cause ambiguous responses (e.g., returning all futures if no creator is specified).
149-163: Check usage ofNewPendingFuturesRequestafter expansionsIf additional fields are later added to
pendingFuturesInput, ensure this function enforces capacity constraints or other validations. Currently, onlypaginationis handled.precompiles/justfile (1)
20-24: Improve clarity in the final ABI formatting process.While the logic for pretty-printing the ABI is correct, consider adding comments explaining each shell command step, especially for new contributors who might be unfamiliar with how
jqmerges JSON objects.precompiles/async/events.go (2)
8-9: Suggest grouping imports more granularly.Although not mandatory, you might consider grouping the standard library imports separately from external or project-specific imports to improve readability.
14-17: Good practice: Provide additional context in the event constant documentation.You have declared
EventCreateFutureto represent theCreateFutureevent. Adding a note in the docstring about the correlation between this event and theIAsynccontract's event definition would aid future maintainers in quickly grasping how these correlate.precompiles/async/tx.go (1)
20-52: Apply the Uber Go style guide for better clarity.
- Use PascalCase for function names and variables if it aligns with your project’s naming conventions and the Uber style guide (for example,
NewMsgAddFuturefor the helper).- Before logging “tx called”, consider adding any contextual information such as block height or transaction hash if it aids debugging.
precompiles/async/IAsync.sol (3)
9-10: Enforce code clarity with inlined documentation.Declaring
IAsync constant IASYNC_CONTRACTis helpful. Short inline comments about its usage ensure maintainers understand where and how it’s used.
31-35: Ensure thesubmitterfield is well-documented.Since
FutureResultincludessubmitterasbytes, clarify whether the submitter can also be an address. If so, consistently handle that across the codebase.
53-55: Readability could be enhanced.
FutureByIdResponseis a simple wrapper over aFuture. Consider in-code doc comments explaining why it differs fromFuturesResponse.tests/cases/async_precompile.go (1)
32-32: Address the TODO commentA TODO note regarding “Implement positive test cases with async sidecar integration” indicates incomplete coverage. Add or update test cases to confirm correct sidecar interactions and asynchronous workflows.
Do you want me to sketch an additional test that simulates sidecar calls?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (45)
tests/testdata/snapshot-base/config/config.toml.tmplis excluded by!tests/testdata/**tests/testdata/snapshot-base/config/genesis.jsonis excluded by!tests/testdata/**tests/testdata/snapshot-base/config/gentx/gentx-1f81f9dfef20b1a6ce69d9e0290828e68f1640cb.jsonis excluded by!tests/testdata/**tests/testdata/snapshot-base/config/node_key.jsonis excluded by!tests/testdata/**tests/testdata/snapshot-base/config/priv_validator_key.jsonis excluded by!tests/testdata/**tests/testdata/snapshot-base/keyring-test/35fae4f60845be13ad8c035bd115ea0abcffe2e0.addressis excluded by!tests/testdata/**tests/testdata/snapshot-base/keyring-test/87d9c3bab0a861ab0487891ac0ad3dcab4a59374.addressis excluded by!tests/testdata/**tests/testdata/snapshot-base/keyring-test/alice.infois excluded by!tests/testdata/**tests/testdata/snapshot-keychain/config/config.toml.tmplis excluded by!tests/testdata/**tests/testdata/snapshot-keychain/config/genesis.jsonis excluded by!tests/testdata/**tests/testdata/snapshot-keychain/config/gentx/gentx-da83b9365dd29a0cea789da7f7cfed5a3c94e909.jsonis excluded by!tests/testdata/**tests/testdata/snapshot-keychain/config/node_key.jsonis excluded by!tests/testdata/**tests/testdata/snapshot-keychain/config/priv_validator_key.jsonis excluded by!tests/testdata/**tests/testdata/snapshot-keychain/keyring-test/112ee6bf4992993e241f64fd538e49ec58b3786b.addressis excluded by!tests/testdata/**tests/testdata/snapshot-keychain/keyring-test/196a5d8b9b615cf017cc5d0d20920b243d172d74.addressis excluded by!tests/testdata/**tests/testdata/snapshot-keychain/keyring-test/1e2614eb15781053d180bd190582a3445c8a2fb7.addressis excluded by!tests/testdata/**tests/testdata/snapshot-keychain/keyring-test/3755bf2db2917c22780984cf79700d40798e911a.addressis excluded by!tests/testdata/**tests/testdata/snapshot-keychain/keyring-test/3a41135fdfd412daf024af6f51201c65ddbca70d.addressis excluded by!tests/testdata/**tests/testdata/snapshot-keychain/keyring-test/a8f44773886b082d9b4c7232cc17e47b387c6dd5.addressis excluded by!tests/testdata/**tests/testdata/snapshot-keychain/keyring-test/bob.infois excluded by!tests/testdata/**tests/testdata/snapshot-keychain/keyring-test/val.infois excluded by!tests/testdata/**tests/testdata/snapshot-keychain/keyring-test/writer.infois excluded by!tests/testdata/**tests/testdata/snapshot-many-users/config/config.toml.tmplis excluded by!tests/testdata/**tests/testdata/snapshot-many-users/config/genesis.jsonis excluded by!tests/testdata/**tests/testdata/snapshot-many-users/config/gentx/gentx-363aed1a0db2e101a04eeef57908956a05f89850.jsonis excluded by!tests/testdata/**tests/testdata/snapshot-many-users/config/node_key.jsonis excluded by!tests/testdata/**tests/testdata/snapshot-many-users/config/priv_validator_key.jsonis excluded by!tests/testdata/**tests/testdata/snapshot-many-users/keyring-test/3d23709c5c3ac8fd5caf326a722f80fbf88feb98.addressis excluded by!tests/testdata/**tests/testdata/snapshot-many-users/keyring-test/45c2e85e71517fc064edd4cb4acd342a37fd9ccd.addressis excluded by!tests/testdata/**tests/testdata/snapshot-many-users/keyring-test/4bdf05e32eb8bcc3beaa0b830fc06e121ac66c48.addressis excluded by!tests/testdata/**tests/testdata/snapshot-many-users/keyring-test/4ef223a16ad3cbf44ff9d8fe8ab2c33e3468af93.addressis excluded by!tests/testdata/**tests/testdata/snapshot-many-users/keyring-test/6c11b09eab8dadfdda77035343bc188e99e54a32.addressis excluded by!tests/testdata/**tests/testdata/snapshot-many-users/keyring-test/6dd9b9a7cc65360687fc46fead33ce8d53eb5df3.addressis excluded by!tests/testdata/**tests/testdata/snapshot-many-users/keyring-test/91b35fdec5e6da58356b585f9d35bfab62ca2806.addressis excluded by!tests/testdata/**tests/testdata/snapshot-many-users/keyring-test/a2cd29ceb012ebd34d521731de6d356b1be6dd7b.addressis excluded by!tests/testdata/**tests/testdata/snapshot-many-users/keyring-test/ac3f01ada9b162875e943a06e3539bf96b12c67c.addressis excluded by!tests/testdata/**tests/testdata/snapshot-many-users/keyring-test/alice.infois excluded by!tests/testdata/**tests/testdata/snapshot-many-users/keyring-test/bob.infois excluded by!tests/testdata/**tests/testdata/snapshot-many-users/keyring-test/c73052a41ccb80b7aa853b90206d4efcc77abac2.addressis excluded by!tests/testdata/**tests/testdata/snapshot-many-users/keyring-test/cbacc7da90e4c84e89cbf6da9fd195c0315b5f25.addressis excluded by!tests/testdata/**tests/testdata/snapshot-many-users/keyring-test/charlie.infois excluded by!tests/testdata/**tests/testdata/snapshot-many-users/keyring-test/dave.infois excluded by!tests/testdata/**tests/testdata/snapshot-many-users/keyring-test/erin.infois excluded by!tests/testdata/**tests/testdata/snapshot-many-users/keyring-test/f93edf435017c754770901758a6447b4220f9298.addressis excluded by!tests/testdata/**tests/testdata/snapshot-many-users/keyring-test/frank.infois excluded by!tests/testdata/**
📒 Files selected for processing (20)
localnet.just(1 hunks)precompiles/act/Types.abi(1 hunks)precompiles/async/IAsync.go(1 hunks)precompiles/async/IAsync.sol(1 hunks)precompiles/async/Types.abi(1 hunks)precompiles/async/abi.json(1 hunks)precompiles/async/async.go(1 hunks)precompiles/async/events.go(1 hunks)precompiles/async/query.go(1 hunks)precompiles/async/tx.go(1 hunks)precompiles/async/types.go(1 hunks)precompiles/justfile(1 hunks)precompiles/precompiles.go(2 hunks)precompiles/slinky/abi.json(1 hunks)precompiles/warden/IWarden.go(1 hunks)precompiles/warden/Types.abi(1 hunks)tests/cases/async_precompile.go(1 hunks)tests/justfile(4 hunks)warden/app/legacy.go(1 hunks)warden/x/async/keeper/query.go(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- precompiles/async/Types.abi
- precompiles/act/Types.abi
- precompiles/warden/Types.abi
- precompiles/slinky/abi.json
🧰 Additional context used
📓 Path-based instructions (12)
warden/x/async/keeper/query.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/cases/async_precompile.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
warden/app/legacy.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
precompiles/precompiles.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/justfile (1)
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
precompiles/async/events.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
precompiles/async/query.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
precompiles/async/tx.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
precompiles/async/async.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
precompiles/warden/IWarden.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
precompiles/async/types.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
precompiles/async/IAsync.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (42)
precompiles/async/types.go (4)
73-85: Validate action.Handler if necessary
In mapFuture, consider whether action.Handler needs validation or whether constraints on its format apply. If so, log or return an appropriate error before constructing the Future.
87-108: Ensure consistency between mapFuture and mapFutureResponse
mapFutureResponse calls mapFuture then proceeds to map votes and results. Ensure all future-related fields remain consistent across these helper functions to prevent partial or mismatched data if the code changes in one function but not the other.
110-121: Avoid partial results on vote mapping failures
In mapVotes, you currently halt on the first error encountered. Consider whether partial success handling is needed (e.g., discarding or logging invalid votes) instead of completely returning an error. If partial success is undesired, this is fine.
131-141: Confirm FutureResult alignment with upstream usage
mapFutureResult returns an empty FutureResult when value == nil. Confirm that downstream logic handles the absence of a result correctly and doesn't incorrectly treat empty results as valid data.
precompiles/async/async.go (4)
66-69: Confirm address strategy
Address() returns a hardcoded precompile address (0x0000...0903). Confirm this address does not conflict with other precompiles and that it follows the project’s addressing strategy.
71-88: Review fallback logic for method ID decoding
In RequiredGas, there is a check for len(input) < 4. This is standard for EVM precompiles. Just confirm that scenarios with incomplete input are properly handled (e.g., returning gas=0 might cause further fallback logic).
90-129: Safe approach to panics and error returns
Run handles out-of-gas errors gracefully via defer evmoscmn.HandleGasError(...). Verify that any new error paths or newly added methods also trigger the same error-handling in case of unexpected internal states (e.g., invalid method name or decoding).
145-148: Logging usage validation
Ensure Logger(ctx) is actually used throughout the code and that any ephemeral logs are passed through this logger, preserving correlation IDs or traceability if needed.
precompiles/async/query.go (3)
21-46: Validate newFutureByIdRequest thoroughly
FutureByIdMethod calls newFutureByIdRequest, which only checks for argument count. Confirm additional business constraints (e.g., futureId > 0) are enforced upstream or if necessary, add them here.
67-92: Guard against nil or malformed response
FuturesMethod gracefully checks if response == nil. Consider similarly validating response fields if partial data from the query server can be returned in edge cases (e.g., missing pagination).
122-147: Distinguish between no pending futures and nil response
PendingFuturesMethod checks for response == nil but also composes an empty object if there are no pending futures. Verify that downstream consumers can differentiate between these scenarios appropriately.
precompiles/async/IAsync.go (1)
1-509: Auto-generated file disclaimer
This file is marked as autogenerated. Manual changes will be overwritten. Unless a critical bug or security issue is identified, avoid modifications and regenerate from the source.
warden/x/async/keeper/query.go (1)
9-11: Confirm pointer semantics of returned Keeper
NewQueryServerImpl returns &keeper. Ensure that the keeper's internal state concurrency usage (if any) is correctly handled when multiple queries are invoked simultaneously, especially if the keeper relies on shared references instead of fully stateless operations.
precompiles/justfile (2)
14-14: Consider verifying if other contracts also need to be included.
You have added a new just generate_artifacts async IAsync target for ABI generation. If there are other async-related contracts that must also be built or tested, please verify that this target remains in sync with the rest of the new code to avoid missing artifact generation for additional contracts.
17-18: Ensure consistent compiler version and usage.
You are explicitly invoking solc --evm-version paris for generating ABI files. If different versions of solc are used elsewhere (such as in CI/CD or local development), this could introduce discrepancies in the generated artifacts.
precompiles/async/events.go (2)
1-2: Package naming aligns well with functionality.
Creating a dedicated async package for asynchronous logic is clear and appropriate. No issues found in these lines.
19-58: Validate event correctness and topic creation.
- Ensure the
typedEventis always fully populated:- If a future chain upgrade or changes to
EventCreateFutureadd or remove fields, ensure the method gracefully handles such modifications.
- If a future chain upgrade or changes to
- The approach to creating topics mirrors the typical approach for indexing in Ethereum logs. This is correct if consistency with other precompiles is desired.
- Consider returning an explicit error if
p.ABI.Events[EventCreateFuture]does not exist (for instance, if the ABI was changed or removed).
precompiles/async/tx.go (2)
1-2: Package name is consistent with usage.
Naming the package async for this new functionality is aligned with the rest of the code.
16-18: Confirm method name alignment with contract function.
AddFutureMethod = "addFuture" suggests this string must match the Solidity interface. Double-check for consistency with IAsync.sol to avoid mismatches at runtime.
precompiles/async/IAsync.sol (10)
1-2: License and pragma directive look good.
No concerns here. You’re adhering to a permissive license (LGPL-3.0) and specifying pragma solidity >=0.8.18;.
6-7: Use a private or internal variable if external references aren’t needed.
address constant IASYNC_PRECOMPILE_ADDRESS = ... is appropriately declared. Ensure no other modules manipulate or rely on this address programmatically.
12-17: Future struct fields are explanatory.
They are well named, and your usage of separate fields for handler and input clarifies the struct’s purpose.
19-23: Enum usage is appropriate.
FutureVoteType enumerates the statuses. This helps reduce magic numbers in your code. No issues found.
25-29: Consider consistent type usage.
Line 27 uses bytes Voter;. If Creator is an address, confirm why Voter is bytes. Possibly you want the same address representation, or maybe you need a more generic form.
37-41: Validate array sizes before appending votes.
FutureResponse holds an array of FutureVote. Confirm in your application logic that large responses or invalid user inputs do not break your code or cause excessive memory usage.
43-46: Pagination field usage is correct.
You have a pattern for returning paginated results. This is consistent with typical Cosmos SDK patterns. No issues found.
48-51: Nice use of composition.
FuturesResponse reuses FutureResponse[], keeping the code DRY.
57-62: Detailed docstrings are good.
You have an author tag, a name, and a dev explanation. Great job.
63-105: Interface definitions look comprehensive and aligned with the rest of your code.
- The placeholders for pagination, address filters, etc., match the approach in your Go code.
- The event
CreateFutureuses indexed parameters properly, facilitating efficient log filtering byfutureIdandcreator.
No major concerns.
tests/cases/async_precompile.go (3)
17-19: Registering the test struct is correct
The init() function registering the test struct using Register(&Test_AsyncPrecompile{}) ensures that the framework can discover and run this test suite.
25-30: Verify concurrency handling on WardenNode startup
Starting the Warden node in a goroutine without a corresponding shutdown sequence might risk resource or port conflicts if multiple test contexts run in parallel. Consider adding graceful termination or cleanup logic for c.w.
Line range hint 33-83: Ensure comprehensive fixture handling and validations
- The test verifies creating and retrieving a future, but does not check error conditions or corner cases (e.g., empty handler string, null input) to ensure robust coverage.
- The test checks for correct event emission but does not verify unsuccessful transactions or revert cases.
Consider extending test scenarios for negative testing and improved code coverage.
localnet.just (1)
4-4: Adding the new async precompile address is consistent
Including "0x0000000000000000000000000000000000000903" aligns with the asynchronous features introduced elsewhere in the pull request. Please confirm that any other environment configurations referencing precompile addresses are updated accordingly.
precompiles/precompiles.go (4)
9-9: New async precompile import aligns with feature integration
Importing asyncprecompile is aligned with the new asynchronous functionality. Ensure that the package remains well-documented and maintained.
14-14: Use of asynckeeper
The import alias asynckeeper clearly differentiates it from other keepers, which is good for readability.
19-23: Extended function signature for async functionality
Expanding NewWardenPrecompiles to include asynckeeper asynckeeper.Keeper is valid. Ensure all call sites properly supply this new dependency and handle the resulting error cases if asynckeeper is unavailable.
68-75: Registration of async precompiles and events
The code correctly registers the new async precompile and its corresponding event. Make sure the versioning in event names (e.g., v1beta1) remains consistent with future updates to keep the contract stable across releases.
tests/justfile (2)
28-28: Maintaining consistency with precompile addresses
Including the same precompile address here ensures alignment across different testing snapshots. Update references in associated scripts or deployments to maintain consistency.
56-56: Ensuring precompiles are applied to snapshots
Appending the new address in .app_state.evm.params.active_static_precompiles is correct. Confirm that subsequent snapshot usage includes or tests the new async precompile in relevant scenarios.
Also applies to: 118-118, 183-183
warden/app/legacy.go (1)
343-348: Ensure correct initialization of the new asynchronous precompile dependency.
The call to wardenprecompiles.NewWardenPrecompiles() now includes app.AsyncKeeper as an extra parameter. Please verify that app.AsyncKeeper has been properly instantiated and is ready for use at this point in the application lifecycle.
precompiles/warden/IWarden.go (1)
239-243: Switching to the current ABI parsing approach is valid.
Using abi.JSON(strings.NewReader(IWardenABI)) is fine. Just confirm that IWardenABI is always up to date and not used in a stale or partially updated state.
precompiles/async/abi.json (1)
1-341: Check for accurate structure alignment of the newly introduced future management ABI.
The JSON structure for the asynchronous precompile ABI appears consistent, but a thorough validation is recommended to confirm that the event, function signature, and data structures (such as FutureByIdResponse, FuturesResponse, etc.) match their corresponding on-chain definitions.
precompiles/async/tx.go
Outdated
| func newMsgAddFuture(args []interface{}, origin common.Address, method *abi.Method) (*acttypes.MsgAddFuture, error) { | ||
| if len(args) != 2 { | ||
| return nil, precommon.WrongArgsNumber{Expected: 2, Got: len(args)} | ||
| } | ||
|
|
||
| authority := precommon.Bech32StrFromAddress(origin) | ||
|
|
||
| var input addFutureInput | ||
| if err := method.Inputs.Copy(&input, args); err != nil { | ||
| return nil, fmt.Errorf("error while unpacking args to keysBySpaceIdInput struct: %w", err) | ||
| } | ||
|
|
||
| return &acttypes.MsgAddFuture{ | ||
| Creator: authority, | ||
| Input: input.Input, | ||
| Handler: input.Handler, | ||
| }, nil | ||
| } | ||
|
|
||
| type addFutureInput struct { | ||
| Handler string | ||
| Input []byte | ||
| } |
There was a problem hiding this comment.
Check argument unpacking for reliability.
While the newMsgAddFuture function appears correct:
- Validate that each argument type matches what the Solidity ABI expects.
- Provide user-friendly error messages explaining how many and which types of arguments were expected.
Ensure your custom error clarifies the signature to the caller:
- return nil, precommon.WrongArgsNumber{Expected: 2, Got: len(args)}
+ return nil, fmt.Errorf("expected 2 arguments (handler string, input bytes), got %d", len(args))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func newMsgAddFuture(args []interface{}, origin common.Address, method *abi.Method) (*acttypes.MsgAddFuture, error) { | |
| if len(args) != 2 { | |
| return nil, precommon.WrongArgsNumber{Expected: 2, Got: len(args)} | |
| } | |
| authority := precommon.Bech32StrFromAddress(origin) | |
| var input addFutureInput | |
| if err := method.Inputs.Copy(&input, args); err != nil { | |
| return nil, fmt.Errorf("error while unpacking args to keysBySpaceIdInput struct: %w", err) | |
| } | |
| return &acttypes.MsgAddFuture{ | |
| Creator: authority, | |
| Input: input.Input, | |
| Handler: input.Handler, | |
| }, nil | |
| } | |
| type addFutureInput struct { | |
| Handler string | |
| Input []byte | |
| } | |
| func newMsgAddFuture(args []interface{}, origin common.Address, method *abi.Method) (*acttypes.MsgAddFuture, error) { | |
| if len(args) != 2 { | |
| return nil, fmt.Errorf("expected 2 arguments (handler string, input bytes), got %d", len(args)) | |
| } | |
| authority := precommon.Bech32StrFromAddress(origin) | |
| var input addFutureInput | |
| if err := method.Inputs.Copy(&input, args); err != nil { | |
| return nil, fmt.Errorf("error while unpacking args to keysBySpaceIdInput struct: %w", err) | |
| } | |
| return &acttypes.MsgAddFuture{ | |
| Creator: authority, | |
| Input: input.Input, | |
| Handler: input.Handler, | |
| }, nil | |
| } | |
| type addFutureInput struct { | |
| Handler string | |
| Input []byte | |
| } |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
precompiles/async/IAsync.sol
Outdated
| ) external returns (uint64 futureId); | ||
|
|
||
| /// @dev Defines a method to query future by id. | ||
| /// @param futureId The pagination details |
There was a problem hiding this comment.
/// @param futureId The future id
precompiles/async/types.go
Outdated
| return *r, nil | ||
| } | ||
|
|
||
| func mapFuture(action types.Future) (Future, error) { |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
precompiles/async/tx.go (1)
20-52: Consider enhancing error handling and logging.While the implementation is solid, consider these improvements:
- Wrap errors with more context using
fmt.Errorf("failed to add future: %w", err)- Add more context to debug logs (e.g., origin address)
- Add validation for stateDB parameter
func (p *Precompile) AddFutureMethod( ctx sdk.Context, origin common.Address, stateDB vm.StateDB, method *abi.Method, args []interface{}, ) ([]byte, error) { + if stateDB == nil { + return nil, fmt.Errorf("stateDB cannot be nil") + } msgServer := actmodulekeeper.NewMsgServerImpl(p.asyncmodulekeeper) message, err := newMsgAddFuture(args, origin, method) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create add future message: %w", err) } p.Logger(ctx).Debug( "tx called", "method", method.Name, "args", args, + "origin", origin.String(), ) response, err := msgServer.AddFuture(ctx, message) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to add future: %w", err) }precompiles/async/types.go (5)
13-18: Consider using pointer fields and JSON tags for consistency.
While storingPagination query.PageRequestas a value works, using a pointer field can help avoid copying large structs. Additionally, ifFuturesInputwill be serialized/deserialized in contexts beyond ABI (e.g., JSON), you may want to add appropriate struct tags for consistency with typical Go patterns.
19-38: Clarify return contract for “FromResponse” methods.
This method both modifies its receiver and returns a newly constructedFuturesResponse. Consider one of the following approaches to align with typical Go style and the Uber Go Guide:
- Return a fresh new struct (avoid modifying the receiver).
- Only mutate the receiver and then return it.
This helps reduce confusion for users about which object they should use next.
39-57: Prevent duplication in multiple “FromResponse” methods.
The code here is nearly identical to the “FuturesResponse.FromResponse.” Consider an unexported helper function that handles the shared logic for building futures slices and pagination. That way, other “FromResponse” implementations can simply invoke the helper.
73-85: Robust error messaging for invalid creator addresses.
When encountering invalid addresses, it might be helpful to include the original address string in the error message (e.g.,%q) for easier debugging.
110-121: Add context to vote mapping errors.
When mapping votes fails, wrapping the error with additional context (e.g., which voter index failed) can expedite debugging.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
precompiles/async/IAsync.sol(1 hunks)precompiles/async/tx.go(1 hunks)precompiles/async/types.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- precompiles/async/IAsync.sol
🧰 Additional context used
📓 Path-based instructions (2)
precompiles/async/tx.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
precompiles/async/types.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (8)
precompiles/async/tx.go (3)
1-18: LGTM! Clean package structure and well-organized imports.
The package structure follows Go best practices with properly grouped imports and clear constant definition.
54-71: Enhance argument validation and error messages.
The current implementation needs improvement in argument validation:
- Add method parameter validation
- Provide more descriptive error messages about expected argument types
func newMsgAddFuture(args []interface{}, origin common.Address, method *abi.Method) (*acttypes.MsgAddFuture, error) {
+ if method == nil {
+ return nil, fmt.Errorf("method cannot be nil")
+ }
+
if len(args) != 2 {
- return nil, precommon.WrongArgsNumber{Expected: 2, Got: len(args)}
+ return nil, fmt.Errorf("expected 2 arguments (handler string, input bytes), got %d", len(args))
}73-76: LGTM! Well-structured input type.
The struct definition is clean and uses appropriate types for its purpose.
precompiles/async/types.go (5)
59-71: Graceful handling of nil “res”.
Currently, if res is nil, the code silently sets an empty FutureByIdResponse. Verify that this is the desired behavior, or consider returning an error to avoid confusion.
87-108: Validate “Result” presence for mapped futures.
Here, you populate votes and result. If the calling code expects a valid result, consider returning an error when the FutureResponse.Result is missing or invalid. This ensures that logic consuming this struct is not silently ignoring an unforeseen nil scenario.
123-129: Ensure correct vote type domain.
Since value.Vote is cast to uint8, confirm the original domain does not exceed the range of a single byte. If a future enhancement allows more vote types, this could silently truncate values.
131-141: Return a typed “nil result” error if needed.
If a FutureResult is required downstream, returning a specialized error when value == nil can prevent confusion. Otherwise, returning an empty struct could mask scenarios where a result is unexpectedly absent.
143-152: Consider returning an error when pagination is unexpected.
If a nil *query.PageResponse is unexpected, returning an empty TypesPageResponse might conceal a server issue. Evaluate whether returning an error is more appropriate.
Summary by CodeRabbit
New Features
addFuture,futureById, andpendingFutures.Bug Fixes
Documentation
Tests
Chores