Conversation
gregorydemay
left a comment
There was a problem hiding this comment.
I did a quick 1st review since I would first like to see the #48 merged to have something working end-to-end.
| type Pubkey = vec nat8; | ||
|
|
||
| // Solana account info. | ||
| type AccountInfo = record { |
There was a problem hiding this comment.
Aha you are right. I actually looked into it and the solana_account::Account type is a scam, it actually doesn't contain all the response fields. I had to change the client to return an AccountUi instance which does contain everything, and can be converted to an Account. The changes have now been reflected in the Candid interface.
There was a problem hiding this comment.
Shouldn't space be mandatory (i.e., a u64 instead of Option<u64>)?
There was a problem hiding this comment.
Good question. I was going off of the UiAccount type for which space is optional but it's true that in their RPC docs it seems like it isn't. I couldn't find much about it, but it looks like maybe this is there for legacy reasons?
I changed AccountInfo so that the space field is not optional, but I'm not sure what's the best way to handle the case when UiAccount.space == None. WDYT?
There was a problem hiding this comment.
I couldn't find much about it, but it looks like maybe this is there for legacy reasons?
It might be for backwards-compatibility reason. It seems that field was introduced form the beginning as pub space: Option<u64> and the JSON RPC API I think mark it on purpose as mandatory.
the case when UiAccount.space == None
Good question, the current implementation shows that it cannot but null. So I think it should be acceptable to panic here, so that we are aware of a problem in case their API changes, since it's declared as not null. Maybe add a comment explaining the reason for that.
lpahlavi
left a comment
There was a problem hiding this comment.
Thanks a lot for the review @gregorydemay! I've addressed your feedback. LMK what you think.
| type Pubkey = vec nat8; | ||
|
|
||
| // Solana account info. | ||
| type AccountInfo = record { |
There was a problem hiding this comment.
Aha you are right. I actually looked into it and the solana_account::Account type is a scam, it actually doesn't contain all the response fields. I had to change the client to return an AccountUi instance which does contain everything, and can be converted to an Account. The changes have now been reflected in the Candid interface.
gregorydemay
left a comment
There was a problem hiding this comment.
Some small comments regarding the API but otherwise starting to look good to me!
There was a problem hiding this comment.
Let's put something more interesting here, for example USDC with EPjFWdd5AufqSSqeM2qN1xzybapC8G4wEGGkZwyTDt1v
| type Pubkey = vec nat8; | ||
|
|
||
| // Solana account info. | ||
| type AccountInfo = record { |
There was a problem hiding this comment.
Shouldn't space be mandatory (i.e., a u64 instead of Option<u64>)?
canister/sol_rpc_canister.did
Outdated
| }; | ||
|
|
||
| // Represents the result of a call to the `getAccountInfo` Solana RPC method. | ||
| type GetAccountInfoResult = variant { Ok : AccountInfo; Err : RpcError }; |
There was a problem hiding this comment.
I think this might be wrong, according to the docs
f the requested account doesn't exist result will be null.
Meaning the OK case should actually be Option<AccountInfo>, no?
There was a problem hiding this comment.
You are right actually, good catch! Done.
lpahlavi
left a comment
There was a problem hiding this comment.
@gregorydemay Thanks a lot for the review! I've incorporated your feedback, LMKWYT!
| type Pubkey = vec nat8; | ||
|
|
||
| // Solana account info. | ||
| type AccountInfo = record { |
There was a problem hiding this comment.
Good question. I was going off of the UiAccount type for which space is optional but it's true that in their RPC docs it seems like it isn't. I couldn't find much about it, but it looks like maybe this is there for legacy reasons?
I changed AccountInfo so that the space field is not optional, but I'm not sure what's the best way to handle the case when UiAccount.space == None. WDYT?
canister/sol_rpc_canister.did
Outdated
| }; | ||
|
|
||
| // Represents the result of a call to the `getAccountInfo` Solana RPC method. | ||
| type GetAccountInfoResult = variant { Ok : AccountInfo; Err : RpcError }; |
There was a problem hiding this comment.
You are right actually, good catch! Done.
| impl From<sol_rpc_types::GetAccountInfoParams> for GetAccountInfoParams { | ||
| fn from(params: sol_rpc_types::GetAccountInfoParams) -> Self { | ||
| let config = if params.is_default_config() { | ||
| None |
There was a problem hiding this comment.
Why not skip this check so that GetAccountInfoConfig is no longer optional?
There was a problem hiding this comment.
Actually I thought that this has the advantage of making the request smaller in case the end-user uses the default config (since it is then just serialized as None). I'm not sure the difference in cost is significant but since it's not much effort on our size I think it makes sense to keep this.
| self.parallel_call( | ||
| "getAccountInfo", | ||
| params, | ||
| self.response_size_estimate(1024 + HEADER_SIZE_LIMIT), |
There was a problem hiding this comment.
why 1024? Is this the actual limit, or just an estimate?
There was a problem hiding this comment.
I checked with 1 request and the response size was 782B so this doesn't seem to far off (we should probably double checks those estimates, like for getSlot, before the final release)
There was a problem hiding this comment.
Thanks for checking this! I created a ticket for this: XC-338.
gregorydemay
left a comment
There was a problem hiding this comment.
Thanks @lpahlavi for this PR!
| self.parallel_call( | ||
| "getAccountInfo", | ||
| params, | ||
| self.response_size_estimate(1024 + HEADER_SIZE_LIMIT), |
There was a problem hiding this comment.
I checked with 1 request and the response size was 782B so this doesn't seem to far off (we should probably double checks those estimates, like for getSlot, before the final release)
lpahlavi
left a comment
There was a problem hiding this comment.
Thanks a lot for the reviews and feedback @gregorydemay and @ninegua!
| impl From<sol_rpc_types::GetAccountInfoParams> for GetAccountInfoParams { | ||
| fn from(params: sol_rpc_types::GetAccountInfoParams) -> Self { | ||
| let config = if params.is_default_config() { | ||
| None |
There was a problem hiding this comment.
Actually I thought that this has the advantage of making the request smaller in case the end-user uses the default config (since it is then just serialized as None). I'm not sure the difference in cost is significant but since it's not much effort on our size I think it makes sense to keep this.
| self.parallel_call( | ||
| "getAccountInfo", | ||
| params, | ||
| self.response_size_estimate(1024 + HEADER_SIZE_LIMIT), |
There was a problem hiding this comment.
Thanks for checking this! I created a ticket for this: XC-338.
## 🤖 New release * `sol_rpc_types`: 0.1.0 * `sol_rpc_canister`: 0.1.0 * `sol_rpc_client`: 0.1.0 <details><summary><i><b>Changelog</b></i></summary><p> ## `sol_rpc_types` <blockquote> ## [0.1.0] - 2025-04-29 ### Added - Add Rustdoc for types related to `getTransaction` ([#71](#71)) - Add support for `getTransaction` RPC method ([#68](#68)) - Add `getBlock` RPC method ([#53](#53)) - Add `sendTransaction` RPC method ([#59](#59)) - Add NOTICE to Apache license ([#60](#60)) - Add `getAccountInfo` RPC method ([#49](#49)) - Add metrics ([#41](#41)) - Add logging crate ([#13](#13)) - Add doc build stage to CI ([#19](#19)) - Add support for override providers ([#12](#12)) ### Changed - Use `canlog_derive` and `canlog` from crates.io ([#84](#84)) - Release pipeline ([#4](#4)) - Default commitment level for `SolRpcClient` ([#77](#77)) - Support method `getBalance` ([#74](#74)) - Rename some enum variants to camel case when serializing ([#72](#72)) - Use SOL RPC canister in `basic_solana` example ([#69](#69)) - Add basic README instructions ([#63](#63)) - Cycles cost ([#52](#52)) - Client builder ([#54](#54)) - Round result from `getSlot` RPC method ([#48](#48)) - Use `canhttp` `multi` feature ([#46](#46)) - Implement getSlot RPC method ([#33](#33)) - Add some tested RPC providers for Solana Mainnet and Devnet ([#15](#15)) - Streamline providers ([#32](#32)) - Add support for API keys ([#10](#10)) - Hard-code SOL RPC providers ([#9](#9)) - Basic Solana wallet example ([#1](#1)) - Initial cargo workspace and build pipeline ([#2](#2)) ### Fixed - Correct Solana cluster for dRPC and Helius providers ([#47](#47)) <!-- generated by git-cliff --> Changelog All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). </blockquote> ## `sol_rpc_canister` <blockquote> ## [0.1.0] - 2025-04-29 ### Added - Add support for `getTransaction` RPC method ([#68](#68)) - Add `getBlock` RPC method ([#53](#53)) - Add `sendTransaction` RPC method ([#59](#59)) - Add NOTICE to Apache license ([#60](#60)) - Add `getAccountInfo` RPC method ([#49](#49)) - Add metrics ([#41](#41)) - Add logging crate ([#13](#13)) - Add support for override providers ([#12](#12)) ### Changed - Use `canlog_derive` and `canlog` from crates.io ([#84](#84)) - Release pipeline ([#4](#4)) - Clean-up TODOs ([#81](#81)) - Support method `getBalance` ([#74](#74)) - Remove http_types module and use external ic-http-types crate ([#73](#73)) - Rename some enum variants to camel case when serializing ([#72](#72)) - Use constant size JSON-RPC request ID ([#62](#62)) - Use method from JSON-RPC request for metric ([#61](#61)) - Cycles cost ([#52](#52)) - Client builder ([#54](#54)) - Round result from `getSlot` RPC method ([#48](#48)) - Use `canhttp` `multi` feature ([#46](#46)) - Implement a method for making generic RPC request ([#39](#39)) - Implement getSlot RPC method ([#33](#33)) - Add some tested RPC providers for Solana Mainnet and Devnet ([#15](#15)) - Streamline providers ([#32](#32)) - Update rust toolchain to 1.85 ([#21](#21)) - Remove unnecessary Storable implementations ([#14](#14)) - Add support for API keys ([#10](#10)) - Hard-code SOL RPC providers ([#9](#9)) - Reproducible build ([#3](#3)) - Initial cargo workspace and build pipeline ([#2](#2)) ### Fixed - Set `maxSupportedTransactionVersion` to zero for end-to-end tests ([#85](#85)) - API keys ([#58](#58)) - End-to-end tests ([#45](#45)) - Correct Solana cluster for dRPC and Helius providers ([#47](#47)) - E2e test with Solana test validator ([#20](#20)) - Create test canister on ICP mainnet ([#8](#8)) <!-- generated by git-cliff --> Changelog All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). </blockquote> ## `sol_rpc_client` <blockquote> ## [0.1.0] - 2025-04-29 ### Added - Add support for `getTransaction` RPC method ([#68](#68)) - Add `getBlock` RPC method ([#53](#53)) - Add `sendTransaction` RPC method ([#59](#59)) - Add NOTICE to Apache license ([#60](#60)) - Add `getAccountInfo` RPC method ([#49](#49)) ### Changed - Release pipeline ([#4](#4)) - Default commitment level for `SolRpcClient` ([#77](#77)) - Support method `getBalance` ([#74](#74)) - Use SOL RPC canister in `basic_solana` example ([#69](#69)) - Rust documentation tests for the `SolRpcClient` ([#65](#65)) - Add basic README instructions ([#63](#63)) - Cycles cost ([#52](#52)) - Client builder ([#54](#54)) - Round result from `getSlot` RPC method ([#48](#48)) - Use `canhttp` `multi` feature ([#46](#46)) - Implement a method for making generic RPC request ([#39](#39)) - Implement getSlot RPC method ([#33](#33)) - Streamline providers ([#32](#32)) - Add support for API keys ([#10](#10)) - Hard-code SOL RPC providers ([#9](#9)) - Initial cargo workspace and build pipeline ([#2](#2)) ### Fixed - Forward calls through wallet canister ([#40](#40)) - E2e test with Solana test validator ([#20](#20)) <!-- generated by git-cliff --> Changelog All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/release-plz/release-plz/). --------- Co-authored-by: gregorydemay <gregory.demay@dfinity.org> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
## 🤖 New release * `sol_rpc_canister`: 0.2.0 <details><summary><i><b>Changelog</b></i></summary><p> <blockquote> ## [0.2.0] - 2025-05-27 ### Added - Add client method to sign a transaction ([#113](#113)) - Add Chainstack RPC provider ([#118](#118)) - Add support for `getSignaturesForAddress` ([#106](#106)) - Add support for `getSignatureStatuses` RPC method ([#96](#96)) - Add support for `getTokenAccountBalance` RPC method ([#90](#90)) - Add support for `getTransaction` RPC method ([#68](#68)) - Add `getBlock` RPC method ([#53](#53)) - Add `sendTransaction` RPC method ([#59](#59)) - Add NOTICE to Apache license ([#60](#60)) - Add `getAccountInfo` RPC method ([#49](#49)) - Add metrics ([#41](#41)) - Add logging crate ([#13](#13)) - Add support for override providers ([#12](#12)) ### Changed - Bump version to 0.2 - Candid NonZeroU8 ([#108](#108)) - Add `RoundingError` to `sol_rpc_types` ([#105](#105)) - Use secure primitive types for `Pubkey`, `Signature` and `Hash` ([#98](#98)) - Add support for `getRecentPrioritizationFees` ([#92](#92)) - Simplify API keys provisioning script ([#89](#89)) - Release v0.1.0 ([#88](#88)) - Use `canlog_derive` and `canlog` from crates.io ([#84](#84)) - Release pipeline ([#4](#4)) - Clean-up TODOs ([#81](#81)) - Support method `getBalance` ([#74](#74)) - Remove http_types module and use external ic-http-types crate ([#73](#73)) - Rename some enum variants to camel case when serializing ([#72](#72)) - Use constant size JSON-RPC request ID ([#62](#62)) - Use method from JSON-RPC request for metric ([#61](#61)) - Cycles cost ([#52](#52)) - Client builder ([#54](#54)) - Round result from `getSlot` RPC method ([#48](#48)) - Use `canhttp` `multi` feature ([#46](#46)) - Implement a method for making generic RPC request ([#39](#39)) - Implement getSlot RPC method ([#33](#33)) - Add some tested RPC providers for Solana Mainnet and Devnet ([#15](#15)) - Streamline providers ([#32](#32)) - Update rust toolchain to 1.85 ([#21](#21)) - Remove unnecessary Storable implementations ([#14](#14)) - Add support for API keys ([#10](#10)) - Hard-code SOL RPC providers ([#9](#9)) - Reproducible build ([#3](#3)) - Initial cargo workspace and build pipeline ([#2](#2)) ### Fixed - Missing `TraceHttp` logs ([#129](#129)) - End-to-end tests for `sendTransaction` ([#104](#104)) - Unit test for `getRecentPrioritizationFees` parameters serialization ([#107](#107)) - Integration test for `verifyApiKey` ([#82](#82)) - Set `maxSupportedTransactionVersion` to zero for end-to-end tests ([#85](#85)) - API keys ([#58](#58)) - End-to-end tests ([#45](#45)) - Correct Solana cluster for dRPC and Helius providers ([#47](#47)) - E2e test with Solana test validator ([#20](#20)) - Create test canister on ICP mainnet ([#8](#8)) ### Removed - Remove default/non-default providers ([#122](#122)) </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/release-plz/release-plz/). --------- Co-authored-by: gregorydemay <gregory.demay@dfinity.org> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
## 🤖 New release * `sol_rpc_canister`: 1.0.0 <details><summary><i><b>Changelog</b></i></summary><p> <blockquote> ## [1.0.0] - 2025-06-12 ### Added - Add support for `transactionDetails=accounts` ([#139](#139)) - Add support for `rewards` parameter for `getBlock` ([#135](#135)) - Add helper methods for request builders ([#136](#136)) - Add client method to sign a transaction ([#113](#113)) - Add Chainstack RPC provider ([#118](#118)) - Add support for `getSignaturesForAddress` ([#106](#106)) - Add support for `getSignatureStatuses` RPC method ([#96](#96)) - Add support for `getTokenAccountBalance` RPC method ([#90](#90)) - Add support for `getTransaction` RPC method ([#68](#68)) - Add `getBlock` RPC method ([#53](#53)) - Add `sendTransaction` RPC method ([#59](#59)) - Add NOTICE to Apache license ([#60](#60)) - Add `getAccountInfo` RPC method ([#49](#49)) - Add metrics ([#41](#41)) - Add logging crate ([#13](#13)) - Add support for override providers ([#12](#12)) ### Changed - Bump to 1.0.0 - Inline request parameters `is_default_config` methods ([#138](#138)) - Release v0.2.0 ([#131](#131)) - Bump version and use a release notes template ([#130](#130)) - Candid NonZeroU8 ([#108](#108)) - Add `RoundingError` to `sol_rpc_types` ([#105](#105)) - Use secure primitive types for `Pubkey`, `Signature` and `Hash` ([#98](#98)) - Add support for `getRecentPrioritizationFees` ([#92](#92)) - Simplify API keys provisioning script ([#89](#89)) - Release v0.1.0 ([#88](#88)) - Use `canlog_derive` and `canlog` from crates.io ([#84](#84)) - Release pipeline ([#4](#4)) - Clean-up TODOs ([#81](#81)) - Support method `getBalance` ([#74](#74)) - Remove http_types module and use external ic-http-types crate ([#73](#73)) - Rename some enum variants to camel case when serializing ([#72](#72)) - Use constant size JSON-RPC request ID ([#62](#62)) - Use method from JSON-RPC request for metric ([#61](#61)) - Cycles cost ([#52](#52)) - Client builder ([#54](#54)) - Round result from `getSlot` RPC method ([#48](#48)) - Use `canhttp` `multi` feature ([#46](#46)) - Implement a method for making generic RPC request ([#39](#39)) - Implement getSlot RPC method ([#33](#33)) - Add some tested RPC providers for Solana Mainnet and Devnet ([#15](#15)) - Streamline providers ([#32](#32)) - Update rust toolchain to 1.85 ([#21](#21)) - Remove unnecessary Storable implementations ([#14](#14)) - Add support for API keys ([#10](#10)) - Hard-code SOL RPC providers ([#9](#9)) - Reproducible build ([#3](#3)) - Initial cargo workspace and build pipeline ([#2](#2)) ### Fixed - Missing `TraceHttp` logs ([#129](#129)) - End-to-end tests for `sendTransaction` ([#104](#104)) - Unit test for `getRecentPrioritizationFees` parameters serialization ([#107](#107)) - Integration test for `verifyApiKey` ([#82](#82)) - Set `maxSupportedTransactionVersion` to zero for end-to-end tests ([#85](#85)) - API keys ([#58](#58)) - End-to-end tests ([#45](#45)) - Correct Solana cluster for dRPC and Helius providers ([#47](#47)) - E2e test with Solana test validator ([#20](#20)) - Create test canister on ICP mainnet ([#8](#8)) ### Removed - Remove default/non-default providers ([#122](#122)) </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/release-plz/release-plz/). --------- Co-authored-by: gregorydemay <gregory.demay@dfinity.org> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: gregorydemay <112856886+gregorydemay@users.noreply.github.com>
(XC-288) Add support for the Solana
getAccountInfoRPC method.