refactor(solana-client): clean up create-and-send-spl-transaction #717
Conversation
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
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
samui-wallet-web | 34a8d77 | Commit Preview URL Branch Preview URL |
Dec 16 2025, 09:28 AM |
beeman
force-pushed
the
beeman/solana-client-consistency
branch
from
38bc4e5 to
bbcf51d
Compare
BundleMonFiles updated (5)
Unchanged files (96)
Total files change -238.83KB -21.73% Groups updated (3)
Final result: ✅ View report in BundleMon website ➡️ |
beeman
force-pushed
the
beeman/solana-client-consistency
branch
4 times, most recently
from
9e86fb4 to
441eeab
Compare
beeman
force-pushed
the
beeman/solana-client-consistency
branch
from
441eeab to
2c49cf1
Compare
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 441eeab in 3 minutes and 0 seconds. Click for details.
- Reviewed
805lines of code in9files - Skipped
0files when reviewing. - Skipped posting
4draft 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. packages/portfolio/src/data-access/use-create-and-send-spl-transaction.tsx:18
- Draft comment:
Removed the 'decimals' parameter; ensure that callers are updated accordingly since decimals is now fetched from mint info in the client. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. packages/solana-client/src/create-and-send-spl-transaction.ts:30
- Draft comment:
Decimals is now computed via fetchMint (line 30–32). Confirm that the mint metadata is always available to avoid unexpected errors. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. packages/solana-client/test/create-spl-transfer-transaction.test.ts:277
- Draft comment:
The test 'should throw error when sender is not a keypair signer' still references a 'sender' property, which is outdated. Update the test to use 'transactionSigner' instead. - Reason this comment was not posted:
Comment was on unchanged code.
4. packages/solana-client/src/spl-token-mint-to.ts:3
- Draft comment:
There's an unnecessary trailing comma in the import statement on line 3. Consider removing the comma after 'getMintToCheckedInstruction' for consistency with the project's style. - 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% This is a very minor stylistic issue - a trailing comma in an import statement. While it's technically correct that there's a trailing comma, this falls under "obvious or unimportant" comments. The rules explicitly state "Do NOT make comments that are obvious or unimportant." A trailing comma in an import is a trivial formatting issue that would typically be caught by a linter or formatter, and doesn't affect functionality. It's also the kind of thing that would be automatically fixed by tools like ESLint or Prettier if the project uses them. This is not a meaningful code change that requires human review. Could this be part of a broader style guide enforcement that's important to the project? Maybe the project doesn't use automated formatters and relies on manual review for style consistency? Even if the project doesn't use automated formatters, this is still an extremely minor and obvious issue. The rules explicitly say not to make obvious or unimportant comments, and a trailing comma in an import statement clearly falls into that category. If style consistency were critical enough to warrant manual review of trailing commas, the project would use automated tooling. This comment should be deleted. It's about a trivial formatting issue (trailing comma in import) that is obvious and unimportant, violating the rule against making such comments.
Workflow ID: wflow_Jtq3tRQQm5G70YZn
You can customize 
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 2c49cf1 in 3 minutes and 26 seconds. Click for details.
- Reviewed
805lines of code in9files - Skipped
0files when reviewing. - Skipped posting
1draft 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. packages/solana-client/test/create-spl-transfer-transaction.test.ts:62
- Draft comment:
The mocks for findAssociatedTokenPda return an empty array, causing destination and source ATAs to be undefined in the instructions. Consider enhancing the mock to return realistic ATA addresses to better simulate production behavior. - 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 tests explicitly assert thatdestination: undefinedandsource: undefinedare passed to the instruction functions (lines 46, 48, 74, 86, 88). This strongly suggests that the empty array return fromfindAssociatedTokenPdais intentional - it's testing the behavior when ATAs are undefined. The comment suggests this is a problem and recommends returning "realistic ATA addresses," but doing so would break the test assertions that explicitly check for undefined values. The comment appears to misunderstand the test's purpose, which is to verify the function works correctly even when ATAs are undefined (perhaps the actual implementation derives them internally). Could the comment be suggesting that having undefined ATAs doesn't match production behavior, and therefore the tests aren't realistic? Maybe in production, findAssociatedTokenPda would always return valid addresses, and testing with undefined is not representative of real usage. Even if that's the intent, the tests are explicitly designed to verify undefined behavior - this is clear from the assertions. If the test author wanted to test with real ATA addresses, they would have mocked different return values and changed the assertions. The comment is suggesting a change that would fundamentally alter what these tests are verifying, without evidence that the current approach is wrong. This falls under "speculative" comments - the comment assumes there's a problem without proof. The comment should be deleted. The tests explicitly assert that undefined values are passed for destination and source ATAs (lines 46, 48, 74, 86, 88), indicating this is intentional test behavior. The comment suggests changing the mock in a way that would break these assertions, without providing evidence that the current approach is incorrect.
Workflow ID: wflow_YwIr3aspqcoaNkTp
You can customize
packages/solana-client/test/create-spl-transfer-transaction.test.ts
Outdated
Show resolved
Hide resolved
beeman
force-pushed
the
beeman/solana-client-consistency
branch
from
2c49cf1 to
872a02a
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 872a02a in 2 minutes and 31 seconds. Click for details.
- Reviewed
806lines of code in9files - Skipped
0files when reviewing. - Skipped posting
4draft 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. packages/solana-client/src/create-spl-transfer-instructions.ts:43
- Draft comment:
Destructuring the results from 'findAssociatedTokenPda' assumes a non‐empty array. If no ATA is found the destructured values become undefined—please document or handle this case explicitly. - 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 comment appears to misunderstand how PDA (Program Derived Address) functions work in Solana. Functions likefindAssociatedTokenPdadon't search for existing accounts - they deterministically compute addresses based on seeds. They should always return a valid address (and typically a bump seed), never an empty array. The destructuring pattern[[sourceATA], [destinationATA]]is taking the first element from each returned array, which is standard for PDA functions that return[address, bump]tuples. This is not a case where the array could be empty - it's a mathematical derivation. The comment is likely incorrect and shows a misunderstanding of Solana's PDA mechanism. I might be wrong about the exact return type offindAssociatedTokenPdawithout seeing its implementation. Perhaps this specific function has different behavior than typical PDA functions. However, the name and usage pattern strongly suggest it's a standard PDA derivation function. Even if I'm not 100% certain about the implementation, the burden of proof should be on the comment to show strong evidence of a problem. The standard behavior of PDA functions in Solana is to always return a deterministic address. Without clear evidence that this function behaves differently, the comment appears to be speculative and based on a misunderstanding. This comment should be deleted. It appears to misunderstand how PDA derivation functions work in Solana - they compute addresses deterministically and don't return empty arrays. The comment is speculative without strong evidence of an actual issue.
2. packages/solana-client/src/create-and-send-spl-transaction.ts:31
- Draft comment:
Good refactoring: computing 'decimals' from mint info removes the need for a separate input and reduces potential inconsistency. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. packages/portfolio/src/data-access/use-create-and-send-spl-transaction.tsx:29
- Draft comment:
Removing the 'decimals' parameter from the mutation options streamlines the API, as decimals are now fetched from the mint info. Ensure that mint info is reliably available to avoid unexpected issues. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. packages/solana-client/test/create-spl-transfer-transaction.test.ts:7
- Draft comment:
The mock for 'findAssociatedTokenPda' returns an empty array, so ATA values become undefined. While tests pass, consider using more realistic mock values to better exercise ATA computation logic. - 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% This comment is suggesting a test improvement - using more realistic mock values instead of an empty array. However, the tests are working as intended and explicitly checking for undefined values. This is not pointing out a bug or required change. The comment starts with "While tests pass, consider..." which is a suggestion, not a required change. According to the rules, I should not keep comments that are purely informative or suggestive unless they identify a clear issue. The comment is about code quality/test quality improvement, not a functional bug. The comment might be valid if the undefined values in the tests indicate that the actual implementation logic for ATA computation isn't being properly tested. If the real code relies onfindAssociatedTokenPdareturning actual values, then mocking it to return an empty array might mean the tests aren't exercising the real code paths. While that's a fair point, the comment explicitly states "While tests pass" which acknowledges the tests are working. The comment is asking to "consider" using better mocks, which is a suggestion for improvement, not identifying a bug. According to the rules, I should not keep comments that are suggestions unless they identify a clear required change. This is a test quality suggestion, not a functional issue. This comment should be deleted. It's a suggestion for test improvement ("consider using more realistic mock values") rather than identifying a required code change or bug. The comment acknowledges the tests pass and is purely advisory about test quality.
Workflow ID: wflow_jLR05nU9VcP44ASc
You can customize
tobeycodes
approved these changes
Dec 15, 2025
beeman
force-pushed
the
beeman/solana-client-consistency
branch
from
872a02a to
34a8d77
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
Simplify the
create-and-send-spl-transactionand clean up the api.Part of #508
Screenshots / Video
Checklist
Important
Refactors SPL transaction creation by removing redundant parameters, simplifying logic, and introducing idempotent ATA creation, with updated tests to ensure correctness.
createAndSendSplTransactionby removingdecimalsparameter and fetching it internally increate-and-send-spl-transaction.ts.createSplTransferInstructionsto handle associated token accounts idempotently increate-spl-transfer-instructions.ts.createGetOrCreateAtaInstructionincreate-get-or-create-ata-instruction.tsfor idempotent ATA creation.create-and-send-spl-transaction.integration.test.tsto reflect refactored logic.create-spl-transfer-transaction.test.tsto cover new behavior and edge cases.findAssociatedTokenPdausage fromcreate-and-send-spl-transaction.tsandspl-token-mint-to.ts.portfolioTxSendMutationOptionsinuse-portfolio-tx-send.tsxto align with refactored SPL transaction logic.This description was created by
for 872a02a. You can customize this summary. It will automatically update as commits are pushed.