Skip to content

feat: add support for getSignaturesForAddress#106

Merged
lpahlavi merged 30 commits intomainfrom
lpahlavi/XC-290-get-signatures-for-address
May 21, 2025
Merged

feat: add support for getSignaturesForAddress#106
lpahlavi merged 30 commits intomainfrom
lpahlavi/XC-290-get-signatures-for-address

Conversation

@lpahlavi
Copy link
Contributor

(XC-290) Add support for the getSignaturesForAddress Solana RPC method.

@lpahlavi lpahlavi marked this pull request as ready for review May 16, 2025 11:09
@lpahlavi lpahlavi requested a review from a team as a code owner May 16, 2025 11:09
Copy link
Contributor

@gregorydemay gregorydemay left a comment

Choose a reason for hiding this comment

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

Thanks @lpahlavi for that PR and sorry for the delay in reviewing it. The code looks good, only some minor comments. The main question is how we could use that method to scrape the transactions of an account programmatically and this is currently less clear to me.

// Maximum transaction signatures to return (between 1 and 1,000).
limit: opt nat16;
// Start searching backwards from this transaction signature. If not provided the search
// starts from the top of the highest max confirmed block.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need a bit more explanation here. From testing it seems that

  1. any signature can be used as a "time anchor", not necessarily a signature for a transaction that uses the pubkey account.
  2. The before parameter is actually crucial to have idempotent responses and hence allow the replicas to reach consensus.

It's not yet clear to me how we could use that method to scrape the transactions targeting a certain account? It's not necessarily something that we want to implement but I think we should have a clear idea on how we would do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I guess I am generally weary of adding too much extra documentation to types which are defined by the Solana RPC API, but you're right that in this case some extra IC-specific notes do make sense. PTAL.

In terms of scraping transactions for an account a I see things like this:

  1. get a recent finalized transaction t (e.g. with getSlot + getBlock)
  2. fetch a batch of transactions for a searching backwards from t, and search no further than the last scraped transaction for a
  3. keep fetching batches starting from the earliest transaction in the last batch, until the size of the batch is less than limit

This should work fine, don't you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I guess I am generally weary of adding too much extra documentation to types which are defined by the Solana RPC API, but you're right that in this case some extra IC-specific notes do make sense. PTAL.

Thanks for adding the doc, LGTM!

This should work fine, don't you think?

Yes! I also don't see another way than getting a finalized transaction to bootstrap and potentially needed to fetch many batches of transactions if a lot of transactions are happening on the targeted account. I think we should store this information somewhere, how about adding this high-level algorithm in the design doc (section Retrieve all transactions)?

Comment on lines 2009 to 2011
.modify_params(|params| {
params.limit = Some(GetSignaturesForAddressLimit::try_from(1).unwrap())
})
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: It would be more idiomatic to have a builder method to change that limit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I actually initially added such a method, and then wondered when exactly it makes sense to add builder methods. Right now it is a little bit random which request parameters we have builder methods for and which ones not... I guess ultimately it could even make sense to have builder methods for all parameters. At least the ones that are somewhat likely to be used, e.g. the ones we actually set in int tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

At least the ones that are somewhat likely to be used, e.g. the ones we actually set in int tests.

Yes currently, that's also my compass. I agree that it may not be super consistent but the main ones should be there. We can always add more if there is demand for it.

Copy link
Contributor Author

@lpahlavi lpahlavi left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your review @gregorydemay! I've addressed/answered all of your comments, PTAL and let me know if you agree.

Comment on lines 2009 to 2011
.modify_params(|params| {
params.limit = Some(GetSignaturesForAddressLimit::try_from(1).unwrap())
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I actually initially added such a method, and then wondered when exactly it makes sense to add builder methods. Right now it is a little bit random which request parameters we have builder methods for and which ones not... I guess ultimately it could even make sense to have builder methods for all parameters. At least the ones that are somewhat likely to be used, e.g. the ones we actually set in int tests.

// Maximum transaction signatures to return (between 1 and 1,000).
limit: opt nat16;
// Start searching backwards from this transaction signature. If not provided the search
// starts from the top of the highest max confirmed block.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I guess I am generally weary of adding too much extra documentation to types which are defined by the Solana RPC API, but you're right that in this case some extra IC-specific notes do make sense. PTAL.

In terms of scraping transactions for an account a I see things like this:

  1. get a recent finalized transaction t (e.g. with getSlot + getBlock)
  2. fetch a batch of transactions for a searching backwards from t, and search no further than the last scraped transaction for a
  3. keep fetching batches starting from the earliest transaction in the last batch, until the size of the batch is less than limit

This should work fine, don't you think?

// Start searching backwards from this transaction. This ensures the two clients get the
// same answer. Otherwise, they might get different results due to one of them including
// transactions from more recent blocks than others.
// Also wait until this transaction is finalized, so that all transactions before it should
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gregorydemay here, we could also consider ignoring the transaction status of returned transactions, since otherwise we will virtually only be able to reach consensus when all returned transactions are finalized. WDYT?

@lpahlavi lpahlavi requested a review from gregorydemay May 20, 2025 15:16
Copy link
Contributor

@gregorydemay gregorydemay left a comment

Choose a reason for hiding this comment

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

Thanks @lpahlavi for all the improvements and explanations!

@lpahlavi lpahlavi merged commit 4e4f787 into main May 21, 2025
12 checks passed
@lpahlavi lpahlavi deleted the lpahlavi/XC-290-get-signatures-for-address branch May 21, 2025 09:45
@github-actions github-actions bot mentioned this pull request May 27, 2025
gregorydemay added a commit that referenced this pull request May 27, 2025
## 🤖 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>
@github-actions github-actions bot mentioned this pull request Jun 12, 2025
gregorydemay added a commit that referenced this pull request Jun 12, 2025
## 🤖 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>
@github-actions github-actions bot mentioned this pull request Jul 31, 2025
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