feat: enable inner_instructions in transaction simulation#229
Closed
chauanhtuan185 wants to merge 2 commits intosolana-foundation:mainfrom
Closed
feat: enable inner_instructions in transaction simulation#229chauanhtuan185 wants to merge 2 commits intosolana-foundation:mainfrom
chauanhtuan185 wants to merge 2 commits intosolana-foundation:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 8d7bdcc in 1 minute and 2 seconds. Click for details.
- Reviewed
72lines of code in2files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. crates/lib/src/error.rs:20
- Draft comment:
New error variant 'TransactionSimulationFailed' is added. Ensure its documentation reflects its specific role in simulation failures with inner instructions. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that the documentation reflects the specific role of a new error variant. This falls under asking the author to ensure something is done, which is against the rules.
2. crates/lib/src/error.rs:129
- Draft comment:
Mapping of TransactionSimulationFailed into RpcError via invalid_request is consistent with the other variants. Verify that downstream consumers handle this case as expected. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. crates/lib/src/transaction/versioned_transaction.rs:151
- Draft comment:
Enabling 'inner_instructions: true' in the simulation config ensures inner CPI logs are captured. Confirm the RPC endpoint supports this flag. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm that the RPC endpoint supports a specific flag. This falls under the rule of not asking the author to confirm or double-check things. Therefore, this comment should be removed.
4. crates/lib/src/transaction/versioned_transaction.rs:158
- Draft comment:
Appending inner instruction logs to the error message enhances debugging. This approach clarifies the simulation failure context. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply states the benefit of a change without prompting any further action or consideration from the PR author.
5. crates/lib/src/transaction/versioned_transaction.rs:182
- Draft comment:
Decoding inner instruction data uses 'unwrap_or_default', which may hide decoding errors. Consider handling failures explicitly instead of defaulting to empty data. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_73SlStFh7d3cWYD8
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Contributor
|
Hey thanks @chauanhtuan185 for the PR, we're currently in the process of an audit so the main branch doesn't have all the latest fixes and changes, you can see it at #223 (name of the pending branch is release/feature-freeze-for-audit) This issue was fixed in that branch :) If you have any other issues, don't hesitate to PR against that branch which will remain open until the audit is completed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Enables
inner_instructionsin transaction simulation to capture detailed CPI logs, providing complete visibility into program execution flow for easier debugging.Problem
Previous simulation errors only showed generic error codes without execution context:
Developers couldn't:
Solution
Changes Made
Enable Inner Instructions (
versioned_transaction.rs)inner_instructions: trueEnhanced Error Response (
versioned_transaction.rs)New Error Type (
error.rs)Result
After This PR
Developers now see:
[1] → [2] → [3]Impact
Testing
Breaking Changes
None. Backward-compatible enhancement that adds information to existing error responses.
Important
Enable detailed CPI logging in transaction simulation for better debugging by capturing inner instructions and enhancing error messages.
inner_instructionsinversioned_transaction.rsfor transaction simulation to capture detailed CPI logs.versioned_transaction.rsto include logs and compute units consumed.TransactionSimulationFailederror type inerror.rsfor detailed simulation failure messages.versioned_transaction.rsto verify inclusion of CPI logs in error responses.This description was created by
for 8d7bdcc. You can customize this summary. It will automatically update as commits are pushed.