Servicer & PocketNetworkClient #9

bryanchriswhite · 2023-09-08T11:32:54Z

NOTE: This will be superseded by #19

* main: fix: add -s nested make calls which populate vars Standardised RPC port across localnet and testnet

adds relay worker tests and relay proto messages

* pokt/miner: removes outdated comment removes unnecessary break in session manager Start adds logger to relayer module relay worker tests passing [wip] relay tests failed successfully [wip] go mod tidy [wip] more protobuf error fixes and cleanup [wip] protobuf fixes moving mining logic to miner module add relay mining logic chore: stub out `rollkitPocketNetworkClient` impl. refactor: move servicer client to client pkg wip - adding in types and harness wip: port cmt-pokt servicer & deps test: update hardcoded test keypairs to secp256k1 chore: adapt keys to cosmos secp256k1 chore: port V1 crypto pkg

bryanchriswhite · 2023-09-12T06:31:58Z

client/pokt/cosmos/local.go

+	clientCtx cosmosClient.Context
+}
+
+func NewLocalCosmosPocketClient() modules.PocketNetworkClient {


@Olshansk #ReviewMe This is the PocketNetworkClient implementation that would be used by the servicer command (i.e. pokrolld "internal" servicer).

Looks good at a high level.

I think that all internal business logic that requires access to state or leads to state transitions should tie into the keeper: #19 (comment)

bryanchriswhite · 2023-09-12T06:33:09Z

client/pokt/cosmos/local.go

+) <-chan types.Maybe[*types.TxResult] {
+	resultCh := make(chan types.Maybe[*types.TxResult], 1)
+
+	// TODO_THIS_COMMIT: provide encoding config via DI (?)


Suggested change

// TODO_THIS_COMMIT: provide encoding config via DI (?)

bryanchriswhite · 2023-09-12T06:34:07Z

client/pokt/cosmos/local.go

+	msg cosmosTypes.Msg,
+) <-chan types.Maybe[*types.TxResult] {
+	// construct tx
+	// TODO_THIS_COMMIT: use DI to get updated client context!


Suggested change

// TODO_THIS_COMMIT: use DI to get updated client context!

bryanchriswhite · 2023-09-12T06:37:33Z

client/servicer/client.go

+
+var _ modules.ServicerClient = &servicerClient{}
+
+type servicerClient struct {


@Olshansk #ReviewMe Not much to review here but wanted to call it out regardless. This is a stub for the implementation of the servicer's RPC client. This would be used by the end client (or test) to request relays from the servicer.

The "RPC client" is fundamentally an "off-chain". In other words a "read" in the context of the query/message server.

I think it should be scaffolded as a query using ignite.

The Query Server [1] has access to the keeper, which can in turn update the SMT.

[1] https://docs.cosmos.network/main/building-modules/query-services

bryanchriswhite · 2023-09-12T06:38:23Z

cmd/poktrolld/cmd/root.go

+			ctx := context.WithValue(cmd.Context(), PoktrollDepInjectorContextKey, injector)
+			cmd.SetContext(ctx)
+
+			// get key name from clientCtx


Suggested change

// get key name from clientCtx

- remove irrelevant comment - add comment re: ensuring key exists - rename `servicer` to `srvcr` to avoid collision with imported package name

bryanchriswhite · 2023-09-12T08:02:10Z

servicer/cmd.go

+}
+
+func servicerCmd(cmd *cobra.Command, args []string) error {
+	injector := di.NewInjector()


@Olshansk #ReviewMe Here we're initializing our dependency injection and subsequently providing the cosmos-sdk dependencies for the localCosmosPocketClient implementation of PocketNetworkClient:

cosmos-sdk/client::Context

cosmos-sdk/client/tx::Factory

cosmos-sdk keybase/keyring key name

I believe the injector is now deprecated :)

bryanchriswhite · 2023-09-12T08:06:48Z

servicer/module.go

+	"poktroll/runtime/di"
+)
+
+type servicer struct {


@Olshansk #ReviewMe This is the servicer module implementation.

ACK.

No need to have a logger, but the other types should be embedded in the servicer module's keeper struct.

bryanchriswhite · 2023-09-12T08:08:38Z

session-manager/module.go

+	"poktroll/x/poktroll/types"
+)
+
+type sessionManager struct {


@Olshansk #ReviewMe This is the SessionManager which is used in the Servicer to pass an observable which emits closed sessions to the Miner.

There will be some iterating we'll be doing here that I think will simplify it quite a bit and make it a lot more stateless.

See the session module I just merged to main.

bryanchriswhite · 2023-09-12T08:13:13Z

utils/file_utils.go

@@ -0,0 +1,77 @@
+package utils


All file_utils functions seem be be unused (i.e. it's save to remove them if we want).

bryanchriswhite · 2023-09-12T13:32:42Z

client/pokt/cosmos/local.go

+	closestKey []byte,
+	closestValueHash []byte,
+	closestSum uint64,
+	// TODO: what type should `claim` be?


Suggested change

// TODO: what type should `claim` be?

bryanchriswhite · 2023-09-12T13:33:16Z

client/pokt/cosmos/local.go

+	closestSum uint64,
+	// TODO: what type should `claim` be?
+	proof *smt.SparseMerkleProof,
+	err error,


@red-0ne why does this receive an error?

bryanchriswhite · 2023-09-12T13:34:41Z

client/pokt/cosmos/remote.go

+)
+
+type remoteCosmosPocketClient struct {
+	//privateKey  *secp256k1.PrivKey


Suggested change

//privateKey *secp256k1.PrivKey

bryanchriswhite · 2023-09-12T13:35:28Z

client/pokt/cosmos/remote.go

+	_ modules.PocketNetworkClient = &remoteCosmosPocketClient{}
+)
+
+type remoteCosmosPocketClient struct {


This is unused at the moment but is an example of what would be required after the servicer becomes external.

bryanchriswhite · 2023-09-12T13:36:02Z

client/pokt/cosmos/remote.go

+	closestKey []byte,
+	closestValueHash []byte,
+	closestSum uint64,
+	// TODO: what type should `claim` be?


Suggested change

// TODO: what type should `claim` be?

Olshansk

@bryanchriswhite I realize this is now deprecated but just left comments in the places you asked to review. I think it was a great first iteration and #19 is getting even closer.

The newer PR is pretty close to a first iteration and I think even the leakage of the txconfig and context can be cleaned up after we get the first version into main.

bryanchriswhite and others added 30 commits September 5, 2023 13:08

chore: port V1 crypto pkg

4dd0dae

chore: adapt keys to cosmos secp256k1

46abdd3

test: update hardcoded test keypairs to secp256k1

4ba8145

wip: port cmt-pokt servicer & deps

966b01c

wip - adding in types and harness

252b9e4

Merge branch 'main' into servicer

d474874

* main: fix: add -s nested make calls which populate vars Standardised RPC port across localnet and testnet

refactor: move servicer client to client pkg

bea1a09

chore: stub out rollkitPocketNetworkClient impl.

eaf5872

chore: port V1 crypto pkg

94ee063

chore: adapt keys to cosmos secp256k1

05f9846

test: update hardcoded test keypairs to secp256k1

3284bd2

wip: port cmt-pokt servicer & deps

4aafbc5

wip - adding in types and harness

e5a3272

refactor: move servicer client to client pkg

bc1f9ce

chore: stub out rollkitPocketNetworkClient impl.

d3ad8e1

chore: generate SubmitClaim message

fe2aabb

add relay mining logic

6660d00

moving mining logic to miner module

3376134

[wip] protobuf fixes

99d5caf

[wip] more protobuf error fixes and cleanup

5d69752

[wip] go mod tidy

7401ed5

[wip] relay tests failed successfully

a8be5bd

relay worker tests passing

1ddcc80

adds logger to relayer module

096ba48

removes unnecessary break in session manager Start

ce7d37c

Merge pull request #6 from pokt-network/miner-3

cc40ea1

removes outdated comment

22c3583

chore: generate SubmitProof message

4cf18ac

Merge pull request #5 from pokt-network/miner-2

270d445

adds relay worker tests and relay proto messages

bryanchriswhite changed the title ~~Servicer & PocketNetworkClient [WIP - DO NOT MERGE]~~ Servicer & PocketNetworkClient Sep 12, 2023

bryanchriswhite commented Sep 12, 2023

View reviewed changes

bryanchriswhite added 5 commits September 12, 2023 09:16

chore: tidy up

9f7fe85

- remove irrelevant comment - add comment re: ensuring key exists - rename `servicer` to `srvcr` to avoid collision with imported package name

chore: consolidate protobuf types & cleanup

807810f

chore: move protobuf ignores to top-level .gitignore

0c593b2

refactor: servicer cmd & DI

dc3da2b

tidy: cleanup comments

01a0821

bryanchriswhite commented Sep 12, 2023

View reviewed changes

bryanchriswhite added 6 commits September 12, 2023 13:57

chore: embed servicer config in app.toml

ed082b7

chore: provide servicer dependencies

ddec58a

chore: setup keybase flags for servicer cmd

7eeef73

chore: use cosmos (cometbft) logger

0267f7e

chore: tidy import aliases

8d8c316

chore: add CONSIDERATION comment

9ad244b

bryanchriswhite commented Sep 12, 2023

View reviewed changes

chore: use cosmos logger

75ea180

bryanchriswhite mentioned this pull request Sep 14, 2023

feat: servicer module implementation #19

Closed

Olshansk reviewed Sep 15, 2023

View reviewed changes

bryanchriswhite closed this Sep 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Servicer & PocketNetworkClient #9

Servicer & PocketNetworkClient #9

bryanchriswhite commented Sep 8, 2023 •

edited

Loading

bryanchriswhite Sep 12, 2023

Olshansk Sep 15, 2023

bryanchriswhite Sep 12, 2023

bryanchriswhite Sep 12, 2023

bryanchriswhite Sep 12, 2023

Olshansk Sep 15, 2023

bryanchriswhite Sep 12, 2023

bryanchriswhite Sep 12, 2023

Olshansk Sep 15, 2023

bryanchriswhite Sep 12, 2023

Olshansk Sep 15, 2023

bryanchriswhite Sep 12, 2023

Olshansk Sep 15, 2023

bryanchriswhite Sep 12, 2023

bryanchriswhite Sep 12, 2023

bryanchriswhite Sep 12, 2023 •

edited

Loading

bryanchriswhite Sep 12, 2023

bryanchriswhite Sep 12, 2023 •

edited

Loading

bryanchriswhite Sep 12, 2023

Olshansk left a comment


		var _ modules.ServicerClient = &servicerClient{}

		type servicerClient struct {

Servicer & PocketNetworkClient #9

Servicer & PocketNetworkClient #9

Conversation

bryanchriswhite commented Sep 8, 2023 • edited Loading

NOTE: This will be superseded by #19

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

bryanchriswhite Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryanchriswhite Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Olshansk left a comment

Choose a reason for hiding this comment

bryanchriswhite commented Sep 8, 2023 •

edited

Loading

bryanchriswhite Sep 12, 2023 •

edited

Loading

bryanchriswhite Sep 12, 2023 •

edited

Loading