add index to GSFA response#9683
Conversation
sam0x17
left a comment
There was a problem hiding this comment.
few nits, looks good, going to run CI
| let slot_entries = self.get_slot_entries(slot, 0)?; | ||
| Ok(slot_entries | ||
| .iter() | ||
| .cloned() |
There was a problem hiding this comment.
now that I'm seeing this, would be much more efficient if we could avoid doing an iter-level clone here, much better not to clone/copy until we find the versioned transaction we are looking for. Would you mind tweaking that?
There was a problem hiding this comment.
Good catch! Switched to .into_iter() which consumes the owned Vec - so we don't need to clone the found transaction in the end.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #9683 +/- ##
========================================
Coverage 82.7% 82.7%
========================================
Files 853 853
Lines 319382 319467 +85
========================================
+ Hits 264145 264269 +124
+ Misses 55237 55198 -39 🚀 New features to boost your workflow:
|
|
Just needs that one change and to pass code coverage looks like |
|
An additional problem I missed: the addition of #[serde(default, skip_serializing_if = "Option::is_none")]
pub transaction_index: Option<u32>; |
sam0x17
left a comment
There was a problem hiding this comment.
make non-breaking change by using optional fields
There was a problem hiding this comment.
semver issue
So right now this PR still adds index: u32 to internal/public structs like:
ConfirmedTransactionWithStatusMeta { …, index: u32 }ConfirmedTransactionStatusWithSignature { …, index: u32 }
That’s an additive field, but in Rust it’s still breaking for downstream users doing struct literals or exhaustive destructuring, so this would break semver. In the very least, we need a change that makes these fields optional + default None.
I am double checking with the team to see if this should be a minor bump or a major bump, but minimally we should make it Option
slot vs transaction_slot nit
In find_address_signatures_for_slot, it currently pushes (slot, signature, transaction_index) using the function parameter slot rather than the transaction_slot read from the iterator. It’s probably fine because of the loop’s break condition, but it would be good to tweak this for clarity and future-proofing.
|
Ok I got some clarity, this would indeed be a major version bump because of the addition of fields on public structs But that is fine, because master is already on 4.0 |
sam0x17
left a comment
There was a problem hiding this comment.
Ok this looks good now, I misspoke about the two last struct changes, we should be good to merge this in 4.x as soon as CI passes
…za-xyz#1660) * Add optional transactionIndex to getSignaturesForAddress response Agave 4.0 (anza-xyz/agave#9683) added the 0 based transaction index inside the block to each signature returned by getSignaturesForAddress. This change exposes the new field on the Kit response type as an optional property so callers can read it where available without breaking against older RPC servers that omit it. * Add transactionIndex to bigint exclusions --------- Co-authored-by: Callum <callum.mcintyre@anza.xyz>
Summary
transactionIndexfield togetTransactionandgetSignaturesForAddressRPC responsesChanges
index: u32field toConfirmedTransactionWithStatusMetaandConfirmedTransactionStatusWithSignaturestructsfind_transaction_in_slotin blockstore to return(VersionedTransaction, u32)tuplefind_address_signatures_for_slotto return index alongside slot and signaturetransaction_indextoRpcConfirmedTransactionStatusWithSignatureandEncodedConfirmedTransactionWithStatusMetaresponse typesTesting
blockstore.rsto verify index values are correctly returned