getTransaction #18
getTransaction #18
Conversation
|
Warning Rate limit exceeded@Dodecahedr0x has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 15 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis pull request updates the transaction handling logic in the core RPC and simnet modules. In the RPC module, the Full trait’s methods—get_transaction and get_signature_statuses—now incorporate new Solana transaction types, robust error handling, and updated return types using BoxFuture. In simnet, a new TransactionWithStatusMeta struct is introduced and the GlobalState struct is enhanced with a history field for tracking transactions, along with improved error capturing and base64 serialization of transaction results. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant S as SurfpoolFullRpc
participant L as Local State/RPC Client
C->>S: get_transaction(signature, config)
S->>S: Decode signature (base58)
alt Decoding Fails
S-->>C: Return error/None
else Decoding Succeeds
S->>L: Retrieve transaction data
L-->>S: Return transaction details or None
S-->>C: Return EncodedConfirmedTransactionWithStatusMeta
end
sequenceDiagram
participant C as Client
participant G as GlobalState
C->>G: Send transaction
G->>G: Process transaction (capture metadata & errors)
G->>G: Create TransactionWithStatusMeta
G->>G: Update history (HashMap)
G-->>C: Return base64 serialized transaction result
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
|
||
| Box::pin(async move { | ||
| // TODO: implement new interfaces in LiteSVM to get all the relevant info | ||
| // needed to return the actual tx, not just some metadata |
There was a problem hiding this comment.
Do you know if this is on the roadmap of LiteSVM?
If not, should we just keep a local cache of the transactions that have been received?
There was a problem hiding this comment.
I'm checking with Aursen and will use a local cache depending on his anwser
|
No clear answer from Aursen yet (he wants to ask Kevin first), so I decided to go for the local tx cache in the mean time |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
crates/core/src/simnet/mod.rs (5)
23-23: Use of HashMap for transaction history.
Storing transactions by signature in a HashMap is straightforward. Keep in mind concurrency implications when reading or writing to it in multi-threaded contexts.
39-45: Tuple struct usage for TransactionWithStatusMeta.
This approach is workable. However, a standard struct with named fields might improve clarity.Consider:
- pub struct TransactionWithStatusMeta(u64, Transaction, TransactionMetadata, Option<TransactionError>); + pub struct TransactionWithStatusMeta { + pub slot: u64, + pub transaction: Transaction, + pub metadata: TransactionMetadata, + pub error: Option<TransactionError>, + }
83-83: Fixed fee calculation.
You are multiplying 5000 by the number of signatures. This might not align with actual cluster fee rules. Use a dynamic approach or retrieve fee calculations from the Solana runtime for better accuracy.
196-196: HashMap::new() initialization.
Straightforward and correct. Consider whether you need an initial capacity if high-volume usage is expected.
203-203: Mempool channel creation.
Using unbounded channels is fine for dev/test environments. Consider bounding them for production for better flow control.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/core/src/rpc/full.rs(2 hunks)crates/core/src/simnet/mod.rs(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run_cargo_checks
- GitHub Check: build
🔇 Additional comments (15)
crates/core/src/simnet/mod.rs (9)
1-1: New base64 import usage is fine.
Bringing in base64 for encoding transaction return data is appropriate and necessary here.
7-7: Check LiteSVM version for transaction metadata methods.
Ensure the currently used LiteSVM version fully supports the newly introduced transaction metadata fields or that any known missing capabilities are tracked as TODO items.
10-15: Relevant Solana SDK imports.
Bringing in clock, epoch info, pubkey, etc., is essential for ledger emulation. All look appropriate. Be sure to keep them in sync with the matching Solana SDK version.
16-21: Extended status imports.
These additional imports for transaction status and UI encoding are in line with the requirement to provide richer transaction metadata.
47-131: Implementation of Into.
The transformation logic comprehensively converts local transaction data into the expected RPC response struct. The repeated field mappings are correct. However, watch out for potential panics (e.g., using unwrap on UTF-8 decoding).
135-135: GlobalState history field addition.
Including the transaction history in GlobalState shows good foresight. Ensure no memory bloat occurs if this is unbounded for very large volumes of transactions.
185-186: Airdrop approach for quick setup.
Calling svm.airdrop is a valid approach for local testing. Confirm that lamports are sufficient for your typical test usage.
321-321: Looping over drained transactions.
Processing all pending transactions within the slot creation loop is a standard approach. Watch out for any hidden performance bottlenecks if large bursts happen.
350-358: Transaction insertion into history.
Storing the transaction outcome in history is helpful for retrieval. Just be mindful of potential repeated writes for the same signature in replays or forks if relevant.crates/core/src/rpc/full.rs (6)
28-28: OptionSerializer usage.
The OptionSerializer import might be part of capturing optional transaction fields. Confirm your approach matches any future changes to the transaction status format.
30-33: Expanded imports for transaction APIs.
These imports are well-aligned with the new getTransaction enhancements.
536-545: RpcEncodingConfigWrapper deserialization block.
This logic for mapping older config wrappers to the newer RpcTransactionConfig is correct. Ensure you handle all relevant edge cases (e.g., unknown encodings).
546-558: Signature decoding from base58.
Rigorous error handling is in place. Good approach. Possibly unify error messages for clarity.
560-569: Local transaction lookup via state_reader.history.
Fetching the transaction from the newly introduced local cache is correct. Just ensure concurrency is managed properly with RwLock usage.
571-588: Graceful fallback to RPC when not found locally.
This fallback ensures a robust approach. If the transaction is missing locally, the method fetches from the cluster. Good coverage of edge cases.
|
Also addresses #27 |
I started implementing the
getTransaction(addressing #26) endpoint but quickly realized that LiteSVM lacks the required methods.Summary by CodeRabbit