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

Add events data to GetTransaction rpc #1192

Closed
wants to merge 5 commits into from
Closed

Conversation

psheth9
Copy link
Contributor

@psheth9 psheth9 commented Feb 6, 2024

More details here --> stellar/stellar-rpc#16

Known limitations

NA

@psheth9 psheth9 force-pushed the update-get-txn-with-events branch from 4b8e79e to 8dcb16a Compare February 7, 2024 21:07
@stellarsaur
Copy link
Contributor

Once ready for review, please close this PR and reopen in soroban-rpc. We could also merge this PR here when ready and cherry pick over to soroban-rpc, but it would be easier to directly merge this feature to soroban-rpc.

@psheth9 psheth9 marked this pull request as ready for review February 8, 2024 01:11
@psheth9 psheth9 requested review from 2opremio and tamirms February 8, 2024 01:11
@psheth9 psheth9 self-assigned this Feb 8, 2024
Copy link
Contributor

@Shaptic Shaptic left a comment

Choose a reason for hiding this comment

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

LGTM! Minor nits below ⬇️

@@ -198,10 +199,33 @@ func (m *MemoryStore) GetTransaction(hash xdr.Hash) (Transaction, bool, StoreRan
if !ok {
return Transaction{}, false, storeRange
}

var tx_meta xdr.TransactionMeta
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: style consistency so let's use camelCase

return Transaction{}, false, storeRange
}

var events [][]byte
Copy link
Contributor

Choose a reason for hiding this comment

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

preallocate since we know the size?

Suggested change
var events [][]byte
events := make([]byte, 0, len(txEvents))

tx, ok, _ := store.GetTransaction(txHash(1, false))
require.True(t, ok)
require.NotNil(t, tx.Events)

Copy link
Contributor

Choose a reason for hiding this comment

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

length check (easier) or content check (harder) would be good here besides not-nil

@psheth9
Copy link
Contributor Author

psheth9 commented Feb 8, 2024

Moving to soroban-rpc

@psheth9 psheth9 closed this Feb 8, 2024
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.

3 participants