search

babylonchain / babylon-finality-gadget

2 stars 2 forks source link

test: add MockBtcClient #45

Closed bap2pecs closed 2 months ago

bap2pecs commented 2 months ago

Summary

https://github.com/babylonchain/babylon-da-sdk/issues/43#issuecomment-2209846568

we want to avoid using BTC mainnet RPCs in tests

this PR sets up the initial structure using DI to mock the BTCClient

Test Plan

make lint
bap2pecs commented 2 months ago

As we are introducing BtcClient, why not pass this interface as the parameter of the constructor of BabylonQueryClient?

we can do that but why is that better? so context is we need to call QueryIsBlockBabylonFinalized in op-node https://github.com/babylonchain/optimism/blob/feat/babylon-rfc/op-node/rollup/finality/finalizer.go#L219-L238 as well as in our e2e test

in our test, we can pass in a mock object. but in OP's codebase, it's a bit hard since we only want to have minimal changes to their codebase.

And, we don't need to implement the mock by ourselves. Instead, we can use the third-party mock lib to generate mocks.

@gitferry do you mean sth like github.com/golang/mock/mockgen?

Some example: https://github.com/babylonchain/staking-indexer/blob/ce71c0a6f792b12abf2c225305fb671dc673b976/btcscanner/btc_scanner.go#L59

what part uses mock in this example?

gitferry commented 2 months ago

So, in the codebase, we should always use the real implementation of the BTC client but the network parameter can be configured, e.g., signet or mainnet. We usually use signet for e2e test.

We use mocked btc client only for unit tests. See https://github.com/babylonchain/staking-indexer/blob/ce71c0a6f792b12abf2c225305fb671dc673b976/btcscanner/btc_scanner_test.go#L48. The mocked code is generated here https://github.com/babylonchain/staking-indexer/blob/623ddf5d7c617502c3a2dcfd79f93eef0d4046b0/Makefile#L67

bap2pecs commented 2 months ago

So, in the codebase, we should always use the real implementation of the BTC client but the network parameter can be configured

the problem is our FP e2e tests use real implementation of the op-node, which uses the BTC clients here.

but we can't mock anything in the op-node

We usually use signet for e2e test.

this doesn't work in our FP e2e test b/c the inserted delegation is default to start from block 0. so if we use signet, we cannot control when the delegation inbounds.

actually if we are already inserting delegations, we shouldn't use signet. it has to be consistent

bap2pecs commented 2 months ago

@gitferry it also feels wrong to me to query signet RPC in CI just for running e2e tests. also, we will meet with the same rate limiting issue which can fail the CI

gitferry commented 2 months ago

the problem is our FP e2e tests use real implementation of the op-node, which uses the BTC clients here. but we can't mock anything in the op-node

OK, I see your point. It's limited by the op-node. Why not also make the sdk client an interface and part of the Finalizer? Maybe I was still missing some context. Would be happy to jump in a call for this when you are OK

it also feels wrong to me to query signet RPC in CI just for running e2e tests. also, we will meet with the same rate limiting issue which can fail the CI

Can we run the signet locally for e2e like this? https://github.com/babylonchain/staking-indexer/blob/d085cbfa868f7b45be9b83e72c18911087fd5773/itest/bitcoind_node_setup.go#L47

SebastianElvis commented 2 months ago

Discussed offline and it seems that if we use interface in the OP side, we will need to change a lot of code there, which is not desired. So we decided to follow the switch approach in this PR

bap2pecs commented 2 months ago

Discussed offline and it seems that if we use interface in the OP side, we will need to change a lot of code there, which is not desired. So we decided to follow the switch approach in this PR

yeah if later we want to get this accepted by the OP core team, I imagine there will be massive refactoring so probably no need to add too much change for now