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

soroban-rpc: Add transactions to memory store #423

Closed

Conversation

2opremio
Copy link
Contributor

@2opremio 2opremio commented Feb 15, 2023

What

Store transactions in the in-memory store. The key change is:

 // MemoryStore is an in-memory store of soroban events.
@@ -46,7 +30,8 @@ type MemoryStore struct {
        // all events occurring within a specific ledger.
        buckets []bucket
        // start is the index of the head in the circular buffer.
-       start uint32
+       start        uint32
+       transactions map[xdr.Hash]Transaction
 }
@@ -15,22 +12,9 @@ type bucket struct {
        ledgerSeq            uint32
        ledgerCloseTimestamp int64
        events               []event
+       // transactions in the memory store belonging to the ledger in the current bucket
+       // this is used for garbage-collecting the global memory store transactions when the bucket is evicted
+       transactionHashes []xdr.Hash
 }

There is also a lot of shuffling and renaming to account for the store not being used exclusively for events.

Why

#409

Known limitations

The store is not used yet (that will happen in an upcoming PR)

@2opremio 2opremio requested a review from tamirms February 15, 2023 14:30
@2opremio 2opremio changed the title Add transactions to memory store soroban-rpc: Add transactions to memory store Feb 15, 2023
@tamirms
Copy link
Contributor

tamirms commented Feb 15, 2023

I think it would be better to have two separate MemoryStore structs (one for transactions and one for events) instead of combining both transactions and events in the same struct.

Because we won't need to join events with transactions since we agreed not to include any events in the getTransaction() endpoint, I don't think it's necessary to have both events and transactions in the same data structure. Also we probably want to configure a different retention window for transactions since they're going to be much more short lived compared to events.

@2opremio
Copy link
Contributor Author

I will try to use generics. I think it should be feasible.

@2opremio
Copy link
Contributor Author

@tamirms I implemented your suggestion at #427

@tamirms
Copy link
Contributor

tamirms commented Feb 16, 2023

@2opremio I like the implementation in #427 and I prefer it to the approach in this PR.

@2opremio 2opremio closed this Feb 16, 2023
@2opremio 2opremio deleted the add-transactions-to-memory-store branch February 16, 2023 22:23
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