Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rpc: Add getConfirmedSignaturesForAddress #9407

Merged

Conversation

CriesofCarrots
Copy link
Contributor

@CriesofCarrots CriesofCarrots commented Apr 9, 2020

Problem

For the history that an API node maintains, it's not possible to fetch the list of transactions that affect a given address. Now that we index signatures by address, the next step would be to return those signatures

Summary of Changes

  • Fix AddressSignatures blockstore column to handle multiple transactions per address-slot (also clean up some iterator indexes in tests so changes are on in one place in the future)
  • Add blockstore apis to return signatures by address (forward-looking to returning full transactions by address)
  • Add getConfirmedSignaturesforAddress rpc endpoint to expose signatures-by-address data

Fixes #9317

@codecov
Copy link

codecov bot commented Apr 9, 2020

Codecov Report

Merging #9407 into master will decrease coverage by 0.0%.
The diff coverage is 83.4%.

@@           Coverage Diff            @@
##           master   #9407     +/-   ##
========================================
- Coverage    81.1%   81.1%   -0.1%     
========================================
  Files         276     276             
  Lines       62223   62351    +128     
========================================
+ Hits        50519   50622    +103     
- Misses      11704   11729     +25     

Copy link
Member

@mvines mvines left a comment

Choose a reason for hiding this comment

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

Needs a API doc update.

I'm not able to sanely parse the name getAddressConfirmedSignatures. WDYT about getConfirmedSignaturesForAddress?

Overall the implementation itself looks great to me though

core/src/rpc.rs Outdated Show resolved Hide resolved
@CriesofCarrots
Copy link
Contributor Author

@mvines , ready for another look.
Currently, both start_slot and end_slot are required, although I did wonder about making it consistent with getConfirmedBlocks, which only requires start_slot. Do you have any opinions on that?

@CriesofCarrots CriesofCarrots changed the title Rpc: Add getAddressConfirmedSignatures Rpc: Add getConfirmedSignaturesForAddress Apr 10, 2020
@mvines
Copy link
Member

mvines commented Apr 10, 2020

@mvines , ready for another look.
Currently, both start_slot and end_slot are required, although I did wonder about making it consistent with getConfirmedBlocks, which only requires start_slot. Do you have any opinions on that?

Only requiring start_slot seems fine, but then we'd want to return the range that was searched in the response so they know where to search next. Maybe we should return the range regardless.

@CriesofCarrots
Copy link
Contributor Author

@mvines , ready for another look.
Only requiring start_slot seems fine, but then we'd want to return the range that was searched in the response so they know where to search next. Maybe we should return the range regardless.

The optional end_slot is more problematic with getConfirmedTransactionsForAddress, which I'm working on now. It needs an optional encoding parameter as well, and the jsonrpc crate doesn't handle multiple optional positional parameters very well.

So I'd like to leave end_slot as-is. In that case, I don't think we should return the range. Sound okay?

mvines
mvines previously approved these changes Apr 10, 2020
docs/src/apps/jsonrpc-api.md Show resolved Hide resolved
@mergify mergify bot dismissed mvines’s stale review April 10, 2020 02:26

Pull request has been modified.

@CriesofCarrots CriesofCarrots added the automerge Merge this Pull Request automatically once CI passes label Apr 10, 2020
@solana-grimes solana-grimes merged commit 91159ea into solana-labs:master Apr 10, 2020
mergify bot pushed a commit that referenced this pull request Apr 10, 2020
automerge

(cherry picked from commit 91159ea)
mergify bot pushed a commit that referenced this pull request Apr 10, 2020
automerge

(cherry picked from commit 91159ea)
solana-grimes pushed a commit that referenced this pull request Apr 10, 2020
solana-grimes pushed a commit that referenced this pull request Apr 10, 2020
@CriesofCarrots CriesofCarrots deleted the add-get-address-signatures branch April 16, 2020 17:38
danpaul000 pushed a commit to danpaul000/solana that referenced this pull request Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add JSON RPC primitives for fetching the list of transactions that affect a given address
3 participants