-
Notifications
You must be signed in to change notification settings - Fork 170
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 test for checkpoint submissions and state change #147
Conversation
4d11aab
to
2cda35e
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! Some minor comments:
Makefile
Outdated
@@ -447,7 +447,8 @@ localnet-build-nodes: | |||
$(DOCKER) run --rm -v $(CURDIR)/.testnets:/data babylonchain/babylond \ | |||
testnet init-files --v 4 -o /data \ | |||
--starting-ip-address 192.168.10.2 --keyring-backend=test \ | |||
--chain-id chain-test | |||
--chain-id chain-test \ | |||
--btc-confirmation-depth 2 |
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.
indentation looks off
|
||
rawCheckpoint := getCheckpoint(t, clients[0], testEpoch) | ||
|
||
if rawCheckpoint.Status != checkpointingtypes.Sealed { |
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.
What will happen if we are on the 2nd epoch, but the checkpoint didn't have time to become Sealed
yet?
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.
test will fail, although this whole test file is stateless, and this point we should be at least in the middle of second epoch and taking into account there is no latency on localhost it would be weird of it would not be sealed at this point.
|
||
if rawCheckpoint.Status != checkpointingtypes.Submitted { | ||
t.Fatalf("Expected checkpoint for epoch %d to be submitted", testEpoch) | ||
} |
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.
Looks nice! Do we also want to test other cases such as the checkpoints being submitted out of order, the spv proof being inserted before the headers etc?
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.
i would say yes, although at this point it is better to have basic cases covered and add new ones incrementally.
t.Fatalf("failed waiting for btc lightclient block") | ||
} | ||
|
||
// Btc light client chain has been extended by 2 blocks, it means that our checkpoint |
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.
Since the confirmation depth is two blocks, maybe we can create a constant which specifies the confirmation depth (e.g. const K
) and create K
number of headers and wait for those headers using a loop.
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.
I was thinking about something like this, but ultimately I prefer to do this in next pr as quicker those tests are running in CI, quicker we have at least basic flow covered.
Add two new integrations tests:
This effectively closes the loop for at least one epoch. I did not add test for finalization (w-deep) as tests start taking a bit time. Next step is a bit refactoring and speed up for tests.