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

getAccountsTransactions.test: Removed sort from streams back transactions for a given block sequence range #5705

Draft
wants to merge 1 commit into
base: staging
Choose a base branch
from

Conversation

j-s-n
Copy link
Contributor

@j-s-n j-s-n commented Jan 17, 2025

Summary

Removed the sort for the accountTransactions since it is already received in order. I regenerated the fixtures and the random hash lexographically sorted ended up swapping the order of the transactions.

Testing Plan

I regenerated fixtures without sort and test passed

Documentation

Does this change require any updates to the Iron Fish Docs (ex. the RPC API
Reference
)? If yes, link a
related documentation pull request for the website.
N/A

Breaking Change

Is this a breaking change? If yes, add notes below on why this is breaking and label it with breaking-change-rpc or breaking-change-sdk.
N/A

@NullSoldier NullSoldier added the breaking-change-rpc A breaking change to the RPC API that should be called out in release notes label Jan 21, 2025
Copy link
Contributor

@hughy hughy left a comment

Choose a reason for hiding this comment

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

I'm wondering if we should sort both accountTransactionHashes and blockTransactionHashes before comparing. This test seems more concerned with all of the transactions being present than it is with all of them being in the same order as they are on the blocks.

@j-s-n
Copy link
Contributor Author

j-s-n commented Jan 22, 2025

I'm wondering if we should sort both accountTransactionHashes and blockTransactionHashes before comparing. This test seems more concerned with all of the transactions being present than it is with all of them being in the same order as they are on the blocks.

loadTransactionsBySequence range returns the transactions in the sequence order, so we can omit the sort since the hard coded blockTransactionHashes are also in sequence order.

@hughy
Copy link
Contributor

hughy commented Jan 22, 2025

I'm wondering if we should sort both accountTransactionHashes and blockTransactionHashes before comparing. This test seems more concerned with all of the transactions being present than it is with all of them being in the same order as they are on the blocks.

loadTransactionsBySequence range returns the transactions in the sequence order, so we can omit the sort since the hard coded blockTransactionHashes are also in sequence order.

Sounds good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change-rpc A breaking change to the RPC API that should be called out in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants