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

Vanilla Babylon client implementation #2

Merged
merged 14 commits into from
Aug 15, 2022
Merged

Vanilla Babylon client implementation #2

merged 14 commits into from
Aug 15, 2022

Conversation

SebastianElvis
Copy link
Member

@SebastianElvis SebastianElvis commented Aug 11, 2022

This PR introduces the vanilla Babylon client implementation.

Specifically, this PR

  • provides the vanilla Babylon client implementation, which supports logging, graceful shutdown, configuration and two sample queries.
  • adds pointer to the Babylon client for the vigilant reporter/submitter struct
  • refactors configuration module to make it more modular
  • updates the sample config yml file w.r.t. new configurations
  • provides some basic documentation on using vigilantes

Technical approach of Babylon client

The Babylon client is implemented upon https://github.com/strangelove-ventures/lens, which is a fully fledged Cosmos SDK client and is recommended by Zaki. It is implemented upon https://github.com/tendermint/tendermint/rpc/client.

This PR includes an example of invoking an existing query stakingtypes.QueryParams and our own query epochingtypes.QueryParams.

Now the dependencies of modules look like:

  • RPC server has access to submitter/reporter
  • Submitter/reporter has access to BTC client and Babylon client

Example execution

image

Future work (marked TODOs in code and will be done in subsequent PRs)

  • Figuring out all configurations of lens' client implementation
  • Supporting HTTPS and GRPC in Babylon server/client
  • Bumping dependency of btcd, btcwallet and btcctl
  • Implementing queries and invocations necessary for vigilante

@SebastianElvis SebastianElvis marked this pull request as ready for review August 12, 2022 05:47
@@ -0,0 +1,52 @@
package bblclient
Copy link

Choose a reason for hiding this comment

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

Is it supposed to be BBL or BBN?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah now it (and many other places) should be BBN. I should have known this earlier 😢 Will fix

Copy link

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

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

Nice, good to have these SDK tools 👍

AccountPrefix: "babylon",
KeyringBackend: "test",
GasAdjustment: 1.2,
GasPrices: "0.01uatom",
Copy link

Choose a reason for hiding this comment

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

Should the deonmination be in Babylon token? Or is this not specific to Gaia / Cosmos Hub?

Copy link
Member Author

Choose a reason for hiding this comment

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

Judging from https://github.com/strangelove-ventures/lens/blob/main/client/config.go#L84-L118, you are right this should be our token. Will change to 0.01ubbn

Comment on lines 125 to 140
func (r *Reporter) requireGetBabylonClient() (*bblclient.Client, error) {
r.babylonClientLock.Lock()
client := r.babylonClient
r.babylonClientLock.Unlock()
if client == nil {
return nil, errors.New("Babylon client is inactive")
}
return client, nil
}

func (r *Reporter) getBabylonClient() *bblclient.Client {
r.babylonClientLock.Lock()
client := r.babylonClient
r.babylonClientLock.Unlock()
return client
}
Copy link

Choose a reason for hiding this comment

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

My prior expectation seeing getBabylonClient and requireGetBabylonClient would be theat they work like Marshal and MustMarshal, that the first might return an error (or in this case nil) while the second can panic. Not sure having the require version return an error is more useful than the default version returning nil, you still have to check in both cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was following https://github.com/btcsuite/btcwallet/blob/master/wallet/wallet.go#L219-L231, but I agree with you that the "get" one should return an error and the "must get" one should panic instead. Given that these functions are not used by existing functions but will be used in the future, let's adapt your approach then

@SebastianElvis SebastianElvis merged commit 2fc6077 into main Aug 15, 2022
@SebastianElvis SebastianElvis deleted the bblclient branch August 15, 2022 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants