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

test: TransactionsProvider implementation of BlockchainProvider2<DB> #10594

Merged
merged 12 commits into from
Aug 29, 2024

Conversation

tcoratger
Copy link
Contributor

Should close #10339

@shekhirin shekhirin changed the title test: test TransactionsProvider implementation of BlockchainProvider2<DB> test: TransactionsProvider implementation of BlockchainProvider2<DB> Aug 28, 2024
@shekhirin shekhirin added A-db Related to the database C-test A change that impacts how or what we test A-blockchain-tree Related to sidechains, reorgs and pending blocks labels Aug 28, 2024
Comment on lines 1494 to 1498
tx_count: if tx_count.end == tx_count.start {
tx_count.start..tx_count.start + 1
} else {
tx_count
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: you can accept impl RangeBounds<u8>, and convert into Range here, so you can explicitly pass TEST_TRANSACTIONS_COUNT..=TEST_TRANSACTIONS_COUNT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this solves the problem... Indeed this if/else is there for the case where BlockRangeParams is default (for about half of the tests) and so the range is 0..0 which is not accepted by the random_block_range method which requires at least 0..1 (which was hardcoded before). That's why I did this trick, I think it's enough right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

if you pass 0..=0 to this method, it should get converted to 0..1 which is basically the same as it was before, but more explicit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just fixed, let me know if this is what you had in mind here

Copy link
Collaborator

@shekhirin shekhirin left a comment

Choose a reason for hiding this comment

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

One last nit, otherwise LGTM! Thank you for going through all these comments 😅

@shekhirin shekhirin added this pull request to the merge queue Aug 29, 2024
@tcoratger
Copy link
Contributor Author

One last nit, otherwise LGTM! Thank you for going through all these comments 😅

No problem, always a pleasure to learn and discuss some tricks :)

Merged via the queue into paradigmxyz:main with commit 0dbab7a Aug 29, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-blockchain-tree Related to sidechains, reorgs and pending blocks A-db Related to the database C-test A change that impacts how or what we test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test TransactionsProvider implementation of BlockchainProvider2<DB>
2 participants