feat: vanilla implementation #1

gitferry · 2022-11-14T04:04:24Z

This PR creates a vanilla implementation of Babylon rpc client with most of the code copied from https://github.com/babylonchain/vigilante/blob/965384cd3a659e63662c7fa417c5ed51c0580a5b/babylonclient/client.go

SebastianElvis

Thanks! No blocker from my side. One minor thing is that could we copy the CI thing here as well?

vitsalis

Nice work! The main comments are around the dependency of this repository to the vigilante one, which I think should not happen.

Also, do you think it would be better if we rename instances of client to rpcclient? I think it would make the point of this repository much clearer.

A README with some details about how this can be built and used would also be appreciated but it can happen on another PR.

vitsalis · 2022-11-14T07:36:53Z

.gitconfig

@@ -0,0 +1,2 @@
+[url "ssh://[email protected]/"]
+	insteadOf = https://github.com/


I would say this does not have a place in this repository. If someone wants to use this replacement rule, we can add instructions on how this can happen on the repository README.

vitsalis · 2022-11-14T07:37:31Z

Makefile

+MOCKGEN_REPO=github.com/golang/mock/mockgen
+MOCKGEN_VERSION=v1.6.0
+MOCKGEN_CMD=go run ${MOCKGEN_REPO}@${MOCKGEN_VERSION}
+


Rules for building would also be useful.

Actually, is there a usecase in which someone uses this client directly from this repository without anything else? Currently, I do not think we have such a usecase so a build rule might not be needed.

In the future, I can envision this client also providing a CLI interface in which someone can interact with a remote Babylon node (although the same can be accomplished through babylond commands so this would be a wrapper).

Agreed. We should implement it as a command line tool just like lens did.

Sounds good! An issue for that would be appreciated

vitsalis · 2022-11-14T07:37:51Z

Makefile

+MOCKGEN_VERSION=v1.6.0
+MOCKGEN_CMD=go run ${MOCKGEN_REPO}@${MOCKGEN_VERSION}
+
+test:


Shouldn't this depend on the mocks getting generated by mock-gen? Not entirely familiar with the mockgen package.

Are you saying we should re-generate mock files every time before we run tests? Not sure about that. It seems we can manually generate mock files once and upload them in the repo so that others do not need to generate them again, just like the protobuf files.

vitsalis · 2022-11-14T07:39:25Z

client/client.go

+		return nil, err
+	}
+	log.Debugf("Babylon key directory: %v", cfg.KeyDirectory)
+	log.Debugf("All Babylon addresses: %v", addrs)


Why do we retrieve the Babylon addresses? Is this some kind of sanity check? As they are not used anywhere else in this function.

It seems like a sanity check. Any thoughts @SebastianElvis?

Reporter uses this address to submit txs to Babylon. The logs and thus retrieving addresses can be omitted though.

Just making sure: Do we need to execute cc.ListAddresses() here? I don't see the addrs being used elsewhere other than logging here.

No need, the accounts are retrieved just for logging.

vitsalis · 2022-11-14T07:40:52Z

client/client.go

+	"fmt"
+	"time"
+
+	"github.com/babylonchain/vigilante/config"


If the vigilante depends on the babylon client and the client depends on the vigilante, this might introduce a cyclic dependency.

vitsalis · 2022-11-14T07:44:41Z

client/modified_lensclient.go

+
+// adapted from https://github.com/strangelove-ventures/lens/blob/v0.5.1/client/chain_client.go#L48-L63
+// The notable difference is parsing the key directory
+// TODO: key directory support for different types of keyring backend


We can add a Github issue for that for easier tracking.

@SebastianElvis Not sure about this description. Could you please revisit this TODO and create an issue for tracking if it is needed?

Sure, created #2

vitsalis · 2022-11-14T07:45:31Z

client/query.go

@@ -0,0 +1,173 @@
+package client


As the client aims to be able to perform all queries related to our modules, maybe we can have different query files for each one of the modules we implement.

vitsalis · 2022-11-14T07:46:25Z

client/query_test.go

@@ -0,0 +1,3 @@
+package client_test
+
+// TODO: mocks on queries for testing reporter/*.go


reporter/.*go does not exist here.

vitsalis · 2022-11-14T07:46:45Z

client/tx_test.go

@@ -0,0 +1,3 @@
+package client_test
+
+// TODO: mocks on txs for testing reporter/*.go


vitsalis · 2022-11-14T07:46:59Z

go.mod

+
+require (
+	github.com/babylonchain/babylon v0.1.0
+	github.com/babylonchain/vigilante v0.1.2


This dependency should not exist imo.

…ules

gitferry · 2022-11-14T09:59:44Z

Thanks, @SebastianElvis and @vitsalis for your comments. Please see the latest changes:

added CI configs
removed dependencies over vigilante -- copied Babylon config from the vigilante
separate queries into different query files according to different Babylon modules.

vitsalis

Looks good, thanks for the updates! Also, please enable CircleCI for the repository (looks like only the admin can do this -- which is probably you since you created the repo 😄 )

vitsalis · 2022-11-14T19:52:56Z

Makefile

+MOCKGEN_REPO=github.com/golang/mock/mockgen
+MOCKGEN_VERSION=v1.6.0
+MOCKGEN_CMD=go run ${MOCKGEN_REPO}@${MOCKGEN_VERSION}
+


Sounds good! An issue for that would be appreciated

vitsalis · 2022-11-14T19:55:39Z

client/query_staking.go

+
+// QueryStakingParams queries staking module's parameters via ChainClient
+// code is adapted from https://github.com/strangelove-ventures/lens/blob/v0.5.1/cmd/staking.go#L128-L149
+func (c *Client) QueryStakingParams() (*stakingtypes.Params, error) {


Why can't we use this?

It is used here

gitferry · 2022-11-15T01:32:25Z

@vitsalis I enabled CI but it failed due to a lack of build rules. Maybe it will work after we add CLI and add build rules in Makefile? I added issue #4 for this.

* add new queries * add rpc * add more rpc * more methods * add comments * remove space * add comments * add another arg

vanilla implementation

a341fec

gitferry requested review from vitsalis and SebastianElvis November 14, 2022 04:04

SebastianElvis approved these changes Nov 14, 2022

View reviewed changes

vitsalis reviewed Nov 14, 2022

View reviewed changes

gitferry added 2 commits November 14, 2022 17:44

remove dependency on vigilante and separate queries for different mod…

fd5c125

…ules

added ci

99e9fd2

gitferry force-pushed the vanilla-client branch from 5cc08b6 to 99e9fd2 Compare November 14, 2022 09:51

vitsalis approved these changes Nov 14, 2022

View reviewed changes

gitferry merged commit 871da1d into dev Nov 15, 2022

gitferry deleted the vanilla-client branch November 15, 2022 01:27

SebastianElvis pushed a commit that referenced this pull request Jan 21, 2024

feat: add btcstaking/finality module queries (#1)

3cf9759

* add new queries * add rpc * add more rpc * more methods * add comments * remove space * add comments * add another arg

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: vanilla implementation #1

feat: vanilla implementation #1

gitferry commented Nov 14, 2022 •

edited

Loading

SebastianElvis left a comment

vitsalis left a comment

vitsalis Nov 14, 2022

vitsalis Nov 14, 2022

vitsalis Nov 14, 2022

gitferry Nov 14, 2022

vitsalis Nov 14, 2022

gitferry Nov 15, 2022

vitsalis Nov 14, 2022

gitferry Nov 14, 2022

vitsalis Nov 14, 2022

gitferry Nov 14, 2022

SebastianElvis Nov 14, 2022

vitsalis Nov 14, 2022

SebastianElvis Nov 14, 2022

vitsalis Nov 14, 2022

vitsalis Nov 14, 2022

gitferry Nov 14, 2022

SebastianElvis Nov 14, 2022

vitsalis Nov 14, 2022

vitsalis Nov 14, 2022

vitsalis Nov 14, 2022

vitsalis Nov 14, 2022

gitferry commented Nov 14, 2022 •

edited

Loading

vitsalis left a comment

vitsalis Nov 14, 2022

vitsalis Nov 14, 2022

gitferry Nov 15, 2022

gitferry commented Nov 15, 2022

		@@ -0,0 +1,2 @@
		[url "ssh://[email protected]/"]
		insteadOf = https://github.com/

		@@ -0,0 +1,3 @@
		package client_test

		// TODO: mocks on queries for testing reporter/*.go

		@@ -0,0 +1,3 @@
		package client_test

		// TODO: mocks on txs for testing reporter/*.go

feat: vanilla implementation #1

feat: vanilla implementation #1

Conversation

gitferry commented Nov 14, 2022 • edited Loading

SebastianElvis left a comment

Choose a reason for hiding this comment

vitsalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitferry commented Nov 14, 2022 • edited Loading

vitsalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitferry commented Nov 15, 2022

gitferry commented Nov 14, 2022 •

edited

Loading

gitferry commented Nov 14, 2022 •

edited

Loading