Add Transaction Inspector Support for Squads Vault Transactions#482
Add Transaction Inspector Support for Squads Vault Transactions#482
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to b5af6c8 in 2 minutes and 54 seconds
More details
- Looked at
480lines of code in3files - Skipped
1files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. app/components/inspector/InspectorPage.tsx:210
- Draft comment:
Review the header conversion in convertVaultTransactionToMessage: the computation of numReadonlySignedAccounts and numReadonlyUnsignedAccounts seems custom. Confirm that subtracting numWritableSigners (and numWritableNonSigners) from numSigners and account length is correct per the vault transaction format. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
The comment raises a valid concern about a complex calculation that could affect transaction validity. However, this appears to be a speculative "are you sure?" type comment rather than pointing out a clear issue. The calculations follow a logical pattern - readonly accounts are total accounts minus writable ones. Without access to the Squads vault transaction format specification, we can't definitively say if this is wrong.
The comment identifies a potentially important correctness issue in transaction handling. Getting these calculations wrong could cause transaction failures.
However, the comment is phrased as a request to "confirm" and "review" rather than pointing out a specific issue. Per the rules, we should not ask authors to verify their intentions or double-check things.
Delete this comment since it's asking for verification rather than pointing out a clear issue. If there was evidence the calculation was wrong, that would be different.
2. app/components/inspector/InspectorPage.tsx:290
- Draft comment:
Consider using a clearer base64 conversion method (for example, Buffer.from(...).toString('base64')) instead of converting via String.fromCharCode.apply with Array.from. This could make the code more readable and robust. - Reason this comment was not posted:
Comment was on unchanged code.
3. app/components/inspector/RawInputCard.tsx:120
- Draft comment:
When parsing account addresses, ensure that the logic doesn't inadvertently treat a base64 encoded string as a valid account address. Consider additional validation (e.g. length check or PublicKey construction) to distinguish between a raw transaction message and a Squads proposal account. - 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 code already has a logical flow - it first tries to parse as account address, and only if that fails or returns no accounts does it try base64 parsing. The error handling seems appropriate. The comment suggests a theoretical issue but doesn't point to any actual problems with the current implementation. The current implementation appears safe and well-structured.
The comment raises a valid security concern about input validation. There could potentially be edge cases where malformed input is misinterpreted.
The code's structure already handles validation appropriately by trying account parsing first, then falling back to base64 only if that fails. Any parsing errors are caught and handled.
The comment should be deleted as it suggests adding validation that is effectively already present in the code's structure and error handling.
4. app/components/inspector/RawInputCard.tsx:137
- Draft comment:
Ensure that the error message 'Input must be base64 encoded or a valid account address' is informative enough for end users; consider providing guidance on expected formats. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
5. app/components/inspector/InspectorPage.tsx:214
- Draft comment:
In convertVaultTransactionToMessage, the header fields are computed using arithmetic on properties like numSigners, numWritableSigners, etc. Please double‐check that these calculations (and setting a zero-filled recentBlockhash) match the expected structure for a valid Solana transaction message. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to double-check calculations and ensure they match the expected structure, which violates the rule against asking the author to confirm or ensure behavior. It doesn't provide a specific suggestion or point out a clear issue.
6. app/components/inspector/RawInputCard.tsx:55
- Draft comment:
Object spread using 'null' is not valid. Instead of spreading '...(signatures.length ? { signatures } : null)', use an empty object (e.g. '...(signatures.length ? { signatures } : {})') to avoid runtime errors. - 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/inspector/RawInputCard.tsx:120
- Draft comment:
The input parsing logic in RawInput first attempts to interpret the input as an account address. This may inadvertently capture inputs that are intended as base64 encoded transaction messages if they are a single token. Consider refining the heuristics to better distinguish between a valid public key and a base64 transaction message. - Reason this comment was not posted:
Comment looked like it was already resolved.
8. app/components/inspector/InspectorPage.tsx:16
- Draft comment:
There is a redundant import from the 'bs58' package: both 'import base58 from "bs58";' and 'import bs58 from "bs58";' are present. Consider removing one of these to avoid confusion. - 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%
While having two imports of the same package with different names could potentially cause confusion, in this case both imports are actively used in different parts of the code. Removing one would require renaming usages in other places. This seems more like a style preference than a real issue.
The comment assumes that duplicate imports are inherently bad, but doesn't consider that they might be intentionally used with different aliases for code clarity or historical reasons.
While consistent naming would be nice, this is a minor style issue at best and both imports are actively used. Making this change would require coordinated updates in multiple places.
This comment should be deleted as it points out a minor style issue where both imports are actually being used, making it not worth the effort to change.
9. package.json:33
- Draft comment:
I noticed that the dependency '@solflare-wallet/utl-sdk' might be a misspelling. If this is intended to be '@solflare-wallet/util-sdk', please update the name to avoid potential issues. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_iuC6Qr1UYHnoEXHY
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.
| /> | ||
| ) : inspectorData ? ( | ||
| isSquadsProposalAccountData(inspectorData) ? ( | ||
| <Suspense fallback={<LoadingCard message="Loading proposal data..." />}> |
There was a problem hiding this comment.
Suspense is being used to wrap SquadsProposalInspectorCard in TransactionInspectorPage, but the SWR configuration within SquadsProposalInspectorCard sets suspense: false. To fully leverage Suspense, consider enabling suspense in SWR or removing the Suspense wrapper to avoid redundant loading states.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on b3e05d9 in 59 seconds
More details
- Looked at
26lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. app/components/inspector/RawInputCard.tsx:181
- Draft comment:
The placeholder text update correctly reflects support for Squads vault transactions. Ensure this change aligns with the API expectations and doesn't conflict with any backend naming conventions. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. app/components/inspector/RawInputCard.tsx:229
- Draft comment:
The instruction update to reference 'vault_transaction' and the URL pattern is clear. Verify that this URL pattern matches the Squads interface and that all related documentation is consistent. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. app/components/inspector/RawInputCard.tsx:181
- Draft comment:
The placeholder text now references 'Squads vault transaction account' instead of 'proposal account'. Please ensure this change aligns with the latest Squads terminology and that users understand the intended input. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. app/components/inspector/RawInputCard.tsx:229
- Draft comment:
The instructions now require adding avault_transactionfrom the Squads URL. Verify that this update follows the current Squads guidelines and that any additional backend processing for vault transactions is in place. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. app/components/inspector/RawInputCard.tsx:181
- Draft comment:
Although the UI text changes reflect the new Squads terminology, there is no change in the underlying transaction parsing logic. The PR description mentions resolving issue 123 (transaction history for deleted accounts not accessible), but this diff does not show modifications to how transactions are retrieved or inspected. Confirm that the necessary functionality is implemented elsewhere. - 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 necessary functionality is implemented elsewhere, which violates the rule against asking for confirmation or ensuring behavior is intended. It does not provide a specific code suggestion or ask for a specific test to be written.
Workflow ID: wflow_BM1XuArHJHEKMZis
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 4cc1bf5 in 1 minute and 43 seconds
More details
- Looked at
691lines of code in11files - Skipped
1files when reviewing. - Skipped posting
16drafted comments based on config settings.
1. app/api/metadata/proxy/__tests__/endpoint.test.ts:16
- Draft comment:
Good use of vi.importActual for wrapping node-fetch, ensuring you still have access to original methods. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. app/api/metadata/proxy/__tests__/endpoint.test.ts:25
- Draft comment:
Using vi.mock for dns and providing a default export for promises lookup is clear; consider adding a comment that it supports future DNS test scenarios. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. app/api/metadata/proxy/__tests__/fetch-resource.spec.ts:56
- Draft comment:
When testing promise rejections, consider directly passing the async function (without wrapping in an arrow) to 'await expect()' for clarity. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
4. app/components/inspector/__tests__/InspectorPage.test.tsx:91
- Draft comment:
Consider importing 'waitFor' from '@testing-library/react' instead of using 'vi.waitFor' for consistency with testing-library patterns. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
5. app/components/inspector/__tests__/InspectorPage.test.tsx:70
- Draft comment:
Good approach for overriding next/navigation mocks; ensure that all edge cases of the new Squads VaultTransaction support are also covered in dedicated tests. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
6. app/features/feature-gate/__tests__/feature-gate.spec.ts:46
- Draft comment:
Tests for feature gate information are concise. Ensure that the error-handling scenario (network error) is comprehensive. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
7. app/features/metadata/__tests__/utils.spec.ts:29
- Draft comment:
Tests for getProxiedUri cover different protocols; environment resetting is handled well here. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
8. app/api/metadata/proxy/__tests__/endpoint.test.ts:29
- Draft comment:
Note: The dns mock returns both a 'default' object with promises.lookup and a top-level 'promises' property. If this duplication is required for compatibility with different import styles, consider adding an inline comment to clarify. - 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 duplication does seem intentional to support different import styles - the file imports _dns and uses dns.promises.lookup. The comment suggests adding documentation but doesn't identify a clear problem or suggest a specific improvement. The code works as intended and the duplication serves a purpose.
The duplication could potentially cause maintenance issues if the mock implementations get out of sync. The comment raises a valid point about code clarity.
However, the duplication is a common pattern when mocking modules that can be imported different ways, and the current implementation is clear and functional. Adding a comment wouldn't materially improve the code.
The comment should be removed as it doesn't identify a clear problem or suggest a specific improvement - it just suggests adding documentation for an intentional pattern.
9. app/components/inspector/__tests__/mocks.ts:1
- Draft comment:
Typo in the comment: 'servialized' should be 'serialized'. - 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.
10. app/components/instruction/lighthouse/__tests__/LighthouseDetailsCard.test.tsx:647
- Draft comment:
Several tests rely on chaining nextElementSibling for assertions. Consider refactoring these queries to use Testing Library's 'within' or more robust queries, which can improve readability and reduce flakiness. - 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.
11. app/api/metadata/proxy/__tests__/endpoint.test.ts:87
- Draft comment:
Typographical error: the comment '// fail on encoded incorrectly input' on line 87 is confusing. Consider rephrasing it to something like '// fail on incorrectly encoded input' for better clarity. - 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.
12. app/api/metadata/proxy/__tests__/fetch-resource.spec.ts:62
- Draft comment:
Typographical error: The error message string in the call to mockRejectOnce contains 'https://path/to/resour.ce over limit: 100'. 'resour.ce' appears to be a typo. Consider changing it to 'resource'. - Reason this comment was not posted:
Comment was on unchanged code.
13. app/api/metadata/proxy/__tests__/ip.spec.ts:40
- Draft comment:
Typo found in the comment on line 40: 'do not throw exceptions forinvalid input to not break the execution flow'. It appears 'forinvalid' is missing a space and should be 'for invalid'. - Reason this comment was not posted:
Comment was on unchanged code.
14. app/components/inspector/__tests__/into-transaction-instruction-from-versioned-message.spec.tsx:34
- Draft comment:
Typo: Consider changing 'should return flatten addressTableLookups from message' to 'should return flattened addressTableLookups from message' for clarity. - 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.
15. app/components/inspector/__tests__/mocks.ts:2
- Draft comment:
Typo detected in the comment on line 2: 'servialized' should be changed to 'serialized'. - Reason this comment was not posted:
Comment was on unchanged code.
16. app/components/instruction/lighthouse/__tests__/LighthouseDetailsCard.test.tsx:647
- Draft comment:
Typographical error: 'followingrows' should be corrected to 'following rows'. - 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_YEl1XWW0Y2G6ezeO
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.
| mockFetchOnce(); | ||
|
|
||
| expect(() => { | ||
| await expect(() => { |
There was a problem hiding this comment.
When testing promise rejections, consider passing the promise directly to expect(...).rejects rather than wrapping it in a function. This improves clarity and conforms with typical async testing patterns.
| await expect(() => { | |
| await expect(fetchResource(uri, headers, 100, 100)).rejects.toThrowError('Unsupported Media Type'); |
|
Closing in favor of #499 |
Description
Adds support for viewing Squads
VaultTransactions in the transaction inspector. This is an additional layer of safety when using Squads, since users will be able to use the Explorer to verify a proposed transaction's contents on an independent platform.Type of change
Screenshots
Inspector InputInstructionssection describing how to get the squads transaction idTesting
Related Issues
Checklist
Additional Notes
Important
Add support for Squads Vault Transactions in the transaction inspector, including new input handling and tests.
VaultTransactioninInspectorPage.tsx, allowing users to verify transactions independently.RawInputCard.tsxto handle Squads transaction account input.InstructionsSection.tsxto include Squads transaction instructions.jestwithvitestin test files likeendpoint.test.tsandfetch-resource.spec.ts.InspectorPage.test.tsx.jest.config.jsandjest.setup.jsforvitestcompatibility.@sqds/multisigdependency inpackage.json.README.mdwith new instructions for Squads transactions.This description was created by
for 4cc1bf5. It will automatically update as commits are pushed.