Fix/sol rpc get account info - #4244
Conversation
…removes error class. Adds `space` to return
There was a problem hiding this comment.
🟡 Changes recommended
The remaining account lookup can still trigger the encoding error, and the SPL tests unintentionally execute the SOL suite.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates Solana account retrieval to use base64 encoding, expose account space, and add SOL/SPL test coverage.
Changes:
- Updates account-info retrieval and response fields.
- Adds shared SOL and SPL account-info tests.
- Covers ATA account behavior.
File summaries
| File | Summary |
|---|---|
packages/crypto-rpc/test/spl.js |
Adds SPL and ATA account-info coverage. |
packages/crypto-rpc/test/sol.js |
Adds shared and SOL-specific tests. |
packages/crypto-rpc/lib/sol/SolRpc.js |
Updates account-info retrieval and result fields. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -963,28 +963,21 @@ export class SolRpc { | |||
| * @returns | |||
| */ | |||
| async getAccountInfo({ address, maxDepth }) { | |||
There was a problem hiding this comment.
bitpay calls getAccountInfo through ChainInterface in isAddressPayable (lib/utils.js:511) and relies on the catch at :549 to return false for ATA addresses. With the throw gone an ATA passes validation as a SOL destination, and SOL sent to an ATA is stuck unless the account gets closed. We need a guard on the bitpay side first, something like reject when space > 0, or keep some way to flag ATAs here.
| effectiveMaxDepth = 0; | ||
| } | ||
| const atas = await this.getTokenAccountsByOwner({ address, skipExistenceCheck: true, maxDepth: effectiveMaxDepth }); | ||
| return { lamports, atas, space: accountInfoResponse.value?.space }; |
There was a problem hiding this comment.
space comes back as a bigint but everything else in this return is Number coerced, including lamports on the same line. Two real problems: JSON.stringify throws on bigint, and 0n is falsy, so any if (!space) check rejects normal wallets since 0 is the typical wallet space. Suggest space: accountInfoResponse.value ? Number(accountInfoResponse.value.space) : undefined.
| } | ||
| throw err; | ||
| const accountInfoResponse = await this.rpc | ||
| .getAccountInfo(address, { encoding: 'base64' }) |
There was a problem hiding this comment.
Nothing here reads .data, but base64 drops the old base58 size limit that was keeping these responses small. A big account now sends its whole data payload on every call. Adding dataSlice: { offset: 0, length: 0 } keeps the fix and the response stays tiny. Same applies to the call at :994.
| const errMsg = err.message.toLowerCase(); | ||
| if (SolKit.isSolanaError(err) && errMsg.includes('json-rpc') && errMsg.includes('should be less than 128 bytes')) { | ||
| // This message can occur when getAccountInfo is called with an SPL address instead of a SOL address | ||
| throw new Error(SOL_ERROR_MESSAGES.ATA_ADD_SENT_INSTEAD_OF_SOL_ADD); |
There was a problem hiding this comment.
ATA_ADD_SENT_INSTEAD_OF_SOL_ADD has no references left after this PR, the throw and the test that used it are both gone. error_messages.js:9 can be deleted, unless we end up keeping the throw per the other comment.
Description
Previously, SolRpc specifically handled one type of error (trying to use getAccountInfo for ATAs). This problem could actually be mitigated by specifying the encoding as base64. This change is necessitated by the need for an account's space.
Tests were updated and added to verify behavior.
Changelog
{ encoding: 'base64' }to Solana RPC getAccountInfo callTesting Notes
Add any helpful notes for reviewers to test your code here.
Checklist