Add 30M gas limit to sudo helper #7527
Conversation
|
I don't think this is complete. If the child thing out of gas's, you get a panic, no? So we have to catch the panic, and return an error. This definitely needs tests to ensure we got the flow right |
|
I think the panic gets caught by the top level defer: https://github.com/osmosis-labs/cosmos-sdk/blob/osmo/v0.47.5/baseapp/baseapp.go#L639-L647 |
|
Thats not true for beginblock/endblock code! And more importantly, CW contracts may appear in things that assume a contract they won't panic. (In swaps, we do catch all panics though) |
|
ahh, I keep forgetting we're calling this in beginblock/endblock. Yeah, prob good to have the same defer here |
|
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
|
Warning Rate Limit Exceeded@AlpinYukseloglu has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 6 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. WalkthroughThe recent update in version Changes
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
Important Notice This PR modifies an in-repo Go module. It is one of:
The dependent Go modules, especially the root one, will have to be Please follow the instructions below:
Please let us know if you need any help. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- osmoutils/cosmwasm/helpers.go (2 hunks)
- osmoutils/cosmwasm/helpers_test.go (1 hunks)
Additional comments: 5
osmoutils/cosmwasm/helpers_test.go (2)
- 24-93: The
TestSudoGasLimitfunction provides a good foundation for testing the gas limit functionality of theSudomethod. However, there are a few areas that could be improved for clarity and robustness:
- Test Case Descriptions: The test case names like "valid contract that consumes less than limit" and "valid contract that consumes more than limit" are clear, but adding a brief comment explaining the expected behavior or outcome for each case could enhance readability.
- Error Handling: The test for the scenario where the contract consumes more gas than the limit correctly expects an error. However, it would be beneficial to also test the specific error message or type to ensure that the error is indeed due to gas exhaustion and not some other issue.
- Gas Consumption Comments: The comments regarding gas consumption (lines 54 and 63) are helpful, but it's important to note that actual gas consumption may vary with changes in the contract or the execution environment. It might be useful to clarify that these are approximate values and may need adjustment based on future contract changes or updates to the gas metering logic.
Overall, the test coverage for the new gas limit functionality is a positive step towards ensuring the reliability and security of contract executions within the
cosmwasmpackage.
- 95-108: The
uploadAndInstantiateContracthelper function is well-implemented and serves its purpose in the test suite. However, consider the following suggestions for improvement:
- Error Messages: When asserting no errors with
s.Require().NoError(err), providing a custom error message can help quickly identify the point of failure if the test fails. For example,s.Require().NoError(err, "Failed to read wasm file")for line 100.- Hardcoded Bech32 Prefix: The Bech32 prefix "osmo" is hardcoded in line 105. While this is acceptable for tests specific to the Osmosis project, it's a good practice to avoid hardcoding values that might change or be different in other contexts. If the prefix is configurable or might vary, consider retrieving it from a central configuration or parameter store.
These refinements can enhance the maintainability and clarity of the test code.
osmoutils/cosmwasm/helpers.go (2)
- 11-11: Introducing the
DefaultContractCallGasLimitconstant is a good practice for managing magic numbers within the codebase. This approach enhances readability and maintainability by centralizing the value, making it easier to adjust in the future if needed. The choice of a 30M gas limit aligns with the PR's objective to establish a safer operational boundary for contract executions.- 124-142: The modifications to the
Sudofunction, including the deferred function to catch panics and the implementation of a gas limit, are well thought out and address the concerns raised in the PR discussion. A few points to consider:
- Panic Handling: The use of a deferred function to catch panics and return a specific error message (line 128) is a robust way to handle out-of-gas scenarios. However, ensure that this approach is consistent with the overall error handling strategy of the application, especially in terms of how panics are logged and reported.
- Gas Consumption Tracking: The logic to consume gas used for calling the contract in the parent context (line 141) is crucial for accurate gas accounting. It's important to verify that this approach correctly reflects the gas consumption in scenarios where the contract execution is complex or involves multiple calls.
Overall, these changes significantly improve the security and reliability of contract executions by preventing unbounded gas consumption.
CHANGELOG.md (1)
- 69-69: The addition of a 30M gas limit to CW pool contract calls is documented clearly, ensuring users are aware of this significant change.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- osmoutils/cosmwasm/helpers.go (2 hunks)
- osmoutils/cosmwasm/helpers_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- osmoutils/cosmwasm/helpers.go
- osmoutils/cosmwasm/helpers_test.go
* add 30M gas limit to sudo helper * add changelog * ensure existing lower limit is not overridden * using min, which is allowed now that we support go 1.21 * start implementing tests * catch panics and add tests * clean up test cases * change error return to generic default and clean up tests --------- Co-authored-by: Nicolas Lara <[email protected]> (cherry picked from commit 1e7132d)
* add 30M gas limit to sudo helper * add changelog * ensure existing lower limit is not overridden * using min, which is allowed now that we support go 1.21 * start implementing tests * catch panics and add tests * clean up test cases * change error return to generic default and clean up tests --------- Co-authored-by: Nicolas Lara <[email protected]> (cherry picked from commit 1e7132d) Co-authored-by: Alpo <[email protected]>
Closes: #7526
What is the purpose of the change
This PR adds a 30M gas limit to sudo calls that use our helper. This can be tightened in the future by requiring callers to specify a gas limit, but in the meantime should serve as an upperbound to minimize the attack surface from unbounded CW contract calls.
Testing and Verifying
Existing tests related to sudo calls pass, and the implementation uses the same gas limit as Cosmwasm has for queries.
No direct tests for this, as it seems all our CW helpers are not directly tested and setting this up will be a meaningful lift. Happy to table this PR until we complete this if people are concerned. Documented this here: #7528
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)Summary by CodeRabbit