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

chore: Replace babylonclient with rpc-client #120

Merged
merged 2 commits into from
Nov 30, 2022
Merged

chore: Replace babylonclient with rpc-client #120

merged 2 commits into from
Nov 30, 2022

Conversation

gitferry
Copy link
Collaborator

This PR removes babylonclient and uses rpc-client instead

@gitferry
Copy link
Collaborator Author

I was wondering why CI failed because of lacking access to rpc-client even though I have added my user ssh key

Copy link
Member

@vitsalis vitsalis left a comment

Choose a reason for hiding this comment

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

Looks good! The CI was failing due to an extra key that took precedence over the key you added. Removed it and the CI passes now

@gitferry
Copy link
Collaborator Author

@vitsalis many thanks!

Copy link
Member

@SebastianElvis SebastianElvis left a comment

Choose a reason for hiding this comment

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

Nice, one more step towards the modular implementation! Left some minor comments.

config/config.go Outdated
@@ -7,6 +7,7 @@ import (
"os"
"path/filepath"

bbnCfg "github.com/babylonchain/rpc-client/config"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bbnCfg "github.com/babylonchain/rpc-client/config"
bbncfg "github.com/babylonchain/rpc-client/config"

I remember Go recommends to use lower case for package names

@@ -82,17 +82,17 @@ func (s *Submitter) Start() {
log.Infof("Successfully created the vigilant submitter")
}

func (s *Submitter) GetBabylonClient() (*babylonclient.Client, error) {
func (s *Submitter) GetBabylonClient() (*bbnclient.Client, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to return a concrete client object, or returning an interface is good enough?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. Thanks!

@@ -20,4 +20,3 @@ submitter-build:
mock-gen:
mkdir -p $(MOCKS_DIR)
$(MOCKGEN_CMD) -source=btcclient/interface.go -package mocks -destination $(MOCKS_DIR)/btcclient.go
$(MOCKGEN_CMD) -source=babylonclient/interface.go -package mocks -destination $(MOCKS_DIR)/babylonclient.go
Copy link
Member

Choose a reason for hiding this comment

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

I somehow feel that the BabylonClient interface should be a part of Vigilante, and every future project that uses Babylon client needs to define its own BabylonClient interface.

In the context of vigilante, the BabylonClient interface could be considered as the expected form of the Babylon client implementation from the vigilante's point of view. Other projects may use different functions of a Babylon client, and thus may have different definitions on this interface. Our rpc-client implementation is expected to implement all of these interfaces, so that it can be fitted into all of these use cases.

Another example would be the expected keeper paradigm in Cosmos SDK and Babylon, where each module defines expected keepers that only have a subset of functions of other keepers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. Thanks!

@gitferry gitferry merged commit 37ff01a into dev Nov 30, 2022
@gitferry gitferry deleted the use-rpc-client branch November 30, 2022 03:56
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.

3 participants