Skip to content

feat: decode ComputeBudget data at inspector#466

Merged
ngundotra merged 5 commits intosolana-foundation:masterfrom
rogaldh:feat/decode-account-data-at-inspector
Feb 27, 2025
Merged

feat: decode ComputeBudget data at inspector#466
ngundotra merged 5 commits intosolana-foundation:masterfrom
rogaldh:feat/decode-account-data-at-inspector

Conversation

@rogaldh
Copy link
Copy Markdown
Contributor

@rogaldh rogaldh commented Feb 19, 2025

PR allows to use ComputeBudgetDetailsCard at the inspector page.
This is achieved by allowing to specify component that would be used to decode the data.


Important

Adds BaseInstructionCard for instruction decoding and integrates ComputeBudgetDetailsCard into the inspector with specific handling for ComputeBudget instructions.

  • Behavior:
    • Introduces BaseInstructionCard in BaseInstructionCard.tsx for decoding and displaying instruction data.
    • Integrates ComputeBudgetDetailsCard into the inspector via InstructionsSection.tsx and InspectorInstructionCard.
    • Handles ComputeBudget instructions specifically, with fallback to UnknownDetailsCard for others.
  • Refactoring:
    • Renames RawDetails to BaseRawDetails and RawParsedDetails to BaseRawParsedDetails.
    • Refactors InstructionCard to use BaseInstructionCard.
  • Testing:
    • Adds tests for ComputeBudgetDetailsCard in ComputeBudgetDetailsCard.spec.tsx.
    • Adds utility tests in into-transaction-instruction-from-versioned-message.spec.tsx.

This description was created by Ellipsis for 159823e. It will automatically update as commits are pushed.

@vercel
Copy link
Copy Markdown

vercel bot commented Feb 19, 2025

@rogaldh is attempting to deploy a commit to the Solana Foundation Team on Vercel.

A member of the Team first needs to authorize it.

@rogaldh rogaldh force-pushed the feat/decode-account-data-at-inspector branch from 9fa573b to 69d7f62 Compare February 19, 2025 20:07
@ngundotra
Copy link
Copy Markdown
Contributor

@ellipsis can you give me a code review?

ellipsis-dev[bot]

This comment was marked as resolved.

ellipsis-dev[bot]

This comment was marked as resolved.

ellipsis-dev[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to 159823e in 4 minutes and 2 seconds

More details
  • Looked at 1015 lines of code in 11 files
  • Skipped 1 files when reviewing.
  • Skipped posting 9 drafted comments based on config settings.
1. app/components/common/BaseInstructionCard.tsx:42
  • Draft comment:
    Typo in comment: 'transcation' should be 'transaction'.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    While the typo is real, comments about typos in code comments are generally not important enough to warrant a PR comment. The typo doesn't affect functionality and fixing it provides minimal value. The rules specifically say not to make comments that are obvious or unimportant.
    The typo could potentially cause confusion for future developers reading the code. Documentation quality is important for maintainability.
    While documentation quality matters, a simple typo that doesn't impede understanding isn't significant enough to warrant a PR comment. The meaning is still clear despite the typo.
    Delete this comment as it points out an unimportant typo that doesn't materially affect code quality or understanding.
2. app/components/instruction/ComputeBudgetDetailsCard.tsx:27
  • Draft comment:
    Consider verifying if the imported 'address' function from 'web3js-experimental' meets the project's consistency requirements.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
3. app/components/common/BaseInstructionCard.tsx:41
  • Draft comment:
    Typo in comment: 'transcation' should be 'transaction'.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    While the typo is real, comments about typos in code comments are generally not important enough to warrant a PR comment. The typo doesn't affect functionality, readability is only minimally impacted, and fixing it is not an essential code change. This feels like unnecessary noise in the PR review.
    The typo could theoretically make the code slightly harder to search for if someone was searching for "transaction". Also, maintaining high quality even in comments shows attention to detail.
    While those points have merit, the benefit of fixing this minor typo doesn't outweigh the cost of the noise it adds to the PR review process. The comment is not about a substantive code issue.
    Delete this comment as it points out a trivial typo in a code comment that doesn't meaningfully impact code quality or functionality.
4. app/components/common/BaseInstructionCard.tsx:111
  • Draft comment:
    Consider returning a consistent tuple from 'ixResult' (e.g. always returning 2 elements) for clarity.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    The comment has merit since inconsistent return types can be a code smell. However, the code is working as intended - it only uses the first element of the tuple, and the second element is only needed in the error case. The inconsistency doesn't cause any actual problems here. The suggestion would be a purely stylistic change without clear benefits.
    The inconsistent return type could indicate poor function design and make future maintenance harder. TypeScript might not catch type errors if the tuple structure changes.
    While the critique has merit, the current implementation is simple and functional. The second tuple element is only meaningful in the error case, so forcing consistency would add unnecessary complexity.
    The comment should be removed. While it suggests a valid code style improvement, the current implementation is clear and functional, and changing it would add complexity without clear benefits.
5. app/components/inspector/__tests__/mocks.ts:1
  • Draft comment:
    Typo in comment: 'servialized' should be 'serialized'.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    While this is technically correct, the rules state we should not make purely informative comments. This is a minor typo in a comment that doesn't affect functionality. The meaning is still clear despite the typo. Comments should focus on code changes that require action.
    The typo could potentially confuse future developers or indicate a lack of attention to detail. Documentation quality is important.
    While documentation quality matters, this is an extremely minor issue that doesn't impact code functionality or maintainability. The meaning remains clear despite the typo.
    Delete this comment as it violates the rule about not making purely informative comments. The typo is too minor to warrant a PR comment.
6. app/components/instruction/ComputeBudgetDetailsCard.tsx:16
  • Draft comment:
    Ensure that using the experimental dependency 'web3js-experimental' (imported on L16) is acceptable for production; consider verifying its stability or exploring alternatives.
  • 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.
7. app/components/common/BaseInstructionCard.tsx:42
  • Draft comment:
    Typographical Error: 'transcation' (line 42) should be corrected to 'transaction'.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    While the comment is technically correct about the typo, it's just a spelling error in a code comment. Code comments are important for documentation, but this minor typo doesn't impact code functionality or significantly impair understanding. The rules state we should not make purely informative comments or comments about obvious/unimportant issues.
    The typo could potentially confuse future developers reading the code. Documentation quality is important for maintainability.
    While documentation is important, this typo is minor enough that most readers would still understand the intent. The comment doesn't warrant blocking or delaying the PR.
    Delete this comment as it points out a trivial typo that doesn't impact code functionality or significantly impair understanding.
8. app/components/inspector/__tests__/mocks.ts:2
  • Draft comment:
    Typographical error: The word 'servialized' appears in the comment and should be corrected to 'serialized'.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    While the typo correction is accurate, it's a very minor issue in a documentation comment. It doesn't affect functionality. The rules state we should only keep comments that require code changes or suggest important code quality improvements. Typos in comments are generally too minor to warrant a PR comment.
    The typo could theoretically cause confusion for future developers reading the documentation. Documentation quality is important for code maintainability.
    While documentation is important, this particular typo is obvious enough that readers would still understand the meaning. The cost of the PR interaction outweighs the minor benefit of fixing this typo.
    Delete this comment as it points out a trivial documentation issue that doesn't warrant a PR interaction.
9. app/components/inspector/utils.ts:14
  • Draft comment:
    Typo/clarity suggestion: The comment on line 14 reads 'dynamic means that lookups are taken based not on staticAccountKeys'. Consider rephrasing it to 'dynamic means that lookups are not taken based on staticAccountKeys' or 'dynamic means that lookups are instead derived from lookupsForAccountKeyIndex' for better clarity.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    The suggested rewording is very minor - it just moves the word "not". While the suggested version might be marginally clearer, the original comment is already understandable and the change is extremely minor. The code itself clearly shows the logic through the if/else branches. Comments about pure documentation/wording typically don't meet the bar of "clearly requiring a code change".
    The comment does point out a potentially confusing wording that could trip up future readers. Moving the "not" could make the meaning slightly more clear.
    While technically true, this is an extremely minor wording issue. The original comment is still understandable and the code itself clearly demonstrates the logic. This doesn't meet the bar of requiring a code change.
    Delete this comment as it suggests only a minor documentation rewording that doesn't materially improve understanding, especially given that the code logic is clear.

Workflow ID: wflow_XE1GcJRp52vwl1Lp


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Copy Markdown
Contributor

@ngundotra ngundotra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks!

@ngundotra ngundotra merged commit 940fdd4 into solana-foundation:master Feb 27, 2025
1 of 2 checks passed
@rogaldh rogaldh deleted the feat/decode-account-data-at-inspector branch March 11, 2025 16:45
@rogaldh rogaldh restored the feat/decode-account-data-at-inspector branch December 25, 2025 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants