-
Notifications
You must be signed in to change notification settings - Fork 753
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
Blockchain client requests #965
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
Please don't merge until Thursday (eventually I will just do the releases itself already on Wednesday and do only the announcements on Thursday, then we can proceed with the other stuff one day earlier, should be ok I guess). |
Yeah, I did not intend to merge anything until we release 😄 I also need to add some stuff here. |
f710f9e
to
f097231
Compare
Rebased this. |
Hi @holgerd77, thanks for the rebase, I intend to keep this WIP until we have integrated the client, then this PR will also remove the blockchain wrapper from the client 👍 |
@jochem-brouwer yeah, this was rather a test if a rebase (in general) still works well with the devp2p integration PR merged, since rebase went through without problems I thought I could as well push hehe 😁 |
f097231
to
6d2f7a0
Compare
Rebased on master. This PR will remove the blockchain wrapper from the client. |
blockchain: add maxBlocks parameter to iterator
6d2f7a0
to
f16177e
Compare
Removing this blockchain wrapper in client is a bit more work than anticipated, so I will open a subsequent PR for that one. Ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Events look good.
I've two comments in the code. Apart from that I would say that the new maxBlocks
option in the iterator()
function as well as the new setHead()
functionality should each get at least one additional test case.
packages/blockchain/src/index.ts
Outdated
@@ -770,6 +772,10 @@ export default class Blockchain implements BlockchainInterface { | |||
lastBlock = block | |||
await onBlock(block, reorg) | |||
blockNumber.iaddn(1) | |||
blocksRanCounter++ | |||
if (maxBlocks && blocksRanCounter == maxBlocks) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things here:
-
I guess this would be the only place in our whole codebase where the double equals would be used 😛, can we update to the standard triple equals?
-
The break condition is unnecessarily late in the loop (correct me if I am wrong), this gets most obvious when ran on an edge case (
maxBlocks
=== 0) where code is executed which shouldn't be executed which could lead to side effects. This should be placed at the top (eventually just directly integrate into the while condition, something likewhile (maxBlocks === undefined || maxBlocks && blocksRanCounter == maxBlocks)
)? Can also be done separately as well though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed in 833264f 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have re-fixed in 5497bc0
Ah yes, you are right, I should've added some tests. Will fix (your comments also), thanks! 😄 |
st.equals(i, 0) | ||
st.end() | ||
} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice tests. 👍
blockchain: add setHead / iterator tests blockchain: remove unnecessary iterator logic
833264f
to
5497bc0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Related: #960
This PR introduces some handy things:
afterBlock
andafterTx
now get access to which block/tx is finished. Blockchainiterator
gets an optional parameter which specifies how many blocks should (at most) be ran. It also adds asetHead
method which allows updating the head of a certain tag, and this is saved in the DB. Related issue: #691.