Integrate ecocredits module #153

robert-zaremba · 2020-11-05T16:12:50Z

Closes: #134

Created Server interface
adding server to app module
implementing Route

codecov · 2020-11-05T16:15:16Z

Codecov Report

Merging #153 (5bb66b2) into master (c1e2235) will increase coverage by 0.37%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master     #153      +/-   ##
==========================================
+ Coverage   65.31%   65.68%   +0.37%     
==========================================
  Files          27       28       +1     
  Lines        1508     1501       -7     
==========================================
+ Hits          985      986       +1     
+ Misses        420      413       -7     
+ Partials      103      102       -1

aaronc

Mostly looks okay but left a few comments

aaronc · 2020-11-05T16:19:15Z

x/ecocredit/server/query_server.go

-	}
-
-	return &classInfo, nil
+	return &classInfo, err


This feels like an unnecessary change. Result should be nil if there's an error

Why? If there is an error, then the result by definition in Go SDK is not specified.
I find it not useful to have the additional 3-4 lines of code just of an err check when the error can be returned directly. I found that google and other Go companies are using this patter across different libraries. Here example in Std lib: https://golang.org/src/strconv/atof.go line 559.

aaronc · 2020-11-05T16:19:25Z

x/ecocredit/server/query_server.go

-	}
-
-	return &ecocredit.QueryBatchInfoResponse{Info: &batchInfo}, nil
+	return &ecocredit.QueryBatchInfoResponse{Info: &batchInfo}, err


aaronc · 2020-11-05T16:25:01Z

x/ecocredit/server/server.go

 	batchInfoTable orm.NaturalKeyTable
 }

-func newServer(storeKey sdk.StoreKey) serverImpl {
+// Server is the the ecocredits implementation of ADR 031 Msg Service
+type Server interface {


Not sure this is desirable. I was trying to not expose this outside of RegisterServices. If we need an interface just inherit MsgServer and QueryServer rather than writing out each method.

It helps with testing. The pattern I'm following is to return interfaces wherever possible and following the excellent proverb:

“Accept interfaces, return structs”

There are many benefits from it, especially if we design different components and will like to do unit-tests. It's also part of SOLID principles.

But you are right, we should reuse MsgServer and QueryServer

Right I was just wanting to make the serverimpl private and force people to use clients

The tests all use clients so they don't need this public

Just for context I actually did exactly what you're doing first btw 😉 but then changed to make it private

Thanks for the explanation. BTW: interface, not an implementation. But probably you are right - with the comments above, we won't need to expose anything about the server (neither the combined interface). I will look at it tomorrow again, meanwhile I added CLI code (query is done, Tx only one command).

I found that we are recreating a server and hardcoding it in local functions (eg in SDK modules in all handler methods). This leads to difficulties or weird designs if we want to test or customize it. Instead, I propose to relay more on tests and configurability. In this commit, I compose a Server inside the AppModule. The AppModule constructor could take a server as a parameter, however I decided to not do it and not expose the default functionality. Module test suites however, will like to customize it. They can do that by creating an AppModule themselves - the struct is public.

aaronc · 2020-11-05T20:15:46Z

Is it useful to do all this stylistic refactoring? I thought the goal was to wire things up and add cli commands not just reorganize stuff a bit and honestly I don't see how this reorganization improves things.

aaronc · 2020-11-05T20:16:40Z

x/ecocredit/module/module.go


 	"github.com/regen-network/regen-ledger/x/ecocredit"
 	"github.com/regen-network/regen-ledger/x/ecocredit/server"
 )

 type AppModule struct {
 	Key sdk.StoreKey
+	Srv server.Server


What value is there in adding this here? Where is it going to need to be publicly used?

I wrote about it on Discord and in the commit message: ff2a903

The whole idea is to allow modules to make it dependency: other modules can mock a server if needed.

In fact the main use-case was to use them in handlers. Since we don't need to implement a Router, then it has less utility - app module doesn't need to hold the server instance any more (at least I don't see other methods where it's needed, RegisterGRPCGatewayRoutes also doesn't use the server).

The only reason I to keep it here is to be able to customize the server within the AppModule.RegisterServices. Since the AppModule has much less utility now, then probably this is not needed. We can parametrize the package ecocredits/RegisterServices method (note, it's not in the server anymore, and it will take Server as an argument) and external tests can use that method instead of AppModule. What do you think?

Right, the app module doesn't need a server instance at all with this design and that's intentional. Maybe I need to document the reasons more but here's my thinking:

other modules shouldn't hold "keepers"/servers directly (according to the WIP ADR 033), instead they use clients which handle permissions correctly. So a direct server instance is undesirable because it breaks ocaps/encapsulation

an AppModule can eventually become sort of a "module descriptor" which instantiates module services but doesn't maintain any of its own state. Module descriptors would only contain config parameters. In RegisterServices, services are constructed and registered but not owned by the app module - rather ownership is transfered to the various routers/service managers.

My preference right now is to leave RegisterServices in server as it was. I originally had things the way you have them here, but I changed it because that felt like it had better encapsulation. Basically with RegisterServices in server, tests don't need to know how services are getting implemented or ever have a concrete server implementation - all of that is hidden so there is proper ocaps provided by the clients.

Does that make sense?

In general I'm leaning in the direction of AppModule being much leaner and tests would mainly need RegisterInterfaces and RegisterServices instead of all the other AppModule stuff currently there.

I'm thinking that a better, less high coupled approach might be something like:

what is types in the SDK and now the root in this approach just has types and maybe something like a ModuleDescriptor with just RegisterInterfaces

server has a lightweight ServerModule which just has RegisterServices + RegisterInterfaces

there are separate client modules wired up separately like CLIClientModule, GRPCGatewayClientModule, etc.

This has a dependency structure where x/{module}/server and x/{module}/client depend on x/{module} and app.go only imports x/{module}/server.ServerModule, and we don't have everything like clients and simulations getting imported into app.go.

This makes sense. I like that. Although I would keep the AppModule name - it describes it's goal very well. x/{module} shouldn't import x/{module}/subpackage.

This makes sense to me too. Some comments:

why not call it ModuleServer instead of ServerModule: it's literally a module's server.

clients could be ModuleCLIClient, ModuleGRPCGatewayClient...

I'm not sure why we have RegisterInterfaces in both ServerModule and ModuleDescriptor. Only once should be enough? My preference goes to the root package.

While CLIClientModule and GRPCGatewayClientModule makes things feel more organized, consistent and structured, it also seems overdone, as e.g. GRPCGatewayClientModule afaics will only ever have one line like:

ecocredits.RegisterQueryHandlerClient(context.Background(), mux, ecocredits.NewQueryClient(clientCtx))

The idea is not to expose the Server interfaces (I'm removing that interface), and the AppModule / ServerModule is not a server - it provides module information / descriptors.

x/ecocredit/module/module.go

aaronc · 2020-11-05T20:18:12Z

x/ecocredit/server/server_test.go

-	RegisterServices(key, configuratorFixture)
-	s := testsuite.NewIntegrationTestSuite(configuratorFixture)
+	cfg := configurator.NewFixture(t, []sdk.StoreKey{key}, addrs)
+	registerServers(key, cfg)


Ditto why refactor this? What are you trying to accomplish?

x/ecocredit/module/module.go

robert-zaremba · 2020-11-05T23:36:33Z

I was planning to merge do CLI in a separate PR. I added Query CLI implementation here, the Tx CLI is work in progress.

aaronc

The biggest issue is updating the usage strings of the cmd's. Those need just a bit of grammar editing.

I have different ideas about module organization, but I can maybe demonstrate that in a PoC. Otherwise, generally looks okay. Thanks @robert-zaremba .

aaronc · 2020-11-05T23:48:40Z

x/ecocredit/client/query.go

+func queryClassInfo() *cobra.Command {
+	return &cobra.Command{
+		Use:   "class_info [class_id]",
+		Short: "Select a class info",


Suggested change

Short: "Select a class info",

Short: "Retrieve credit class info",

aaronc · 2020-11-05T23:49:31Z

x/ecocredit/client/query.go

+
+		Args:  cobra.ExactArgs(1),
+		Use:   ecocredit.ModuleName,
+		Short: "Querying commands for the ecocredit module",


Suggested change

Short: "Querying commands for the ecocredit module",

Short: "Query commands for the ecocredit module",

aaronc · 2020-11-05T23:50:42Z

x/ecocredit/client/tx.go

+		Short: `Creates a new credit class.
+  designer:  address of the account which designed the credit class
+  issuer:    comma separated (no spaces) list of issuer account addresses. Example: "addr1,addr2"
+  metadata:  base64 encoded metadata - arbitrary data attached to the credit class info`,


So what do you think about using flags --binary-metadata and --string-metadata? Often it will be json I imagine

x/ecocredit/module/module.go

aaronc · 2020-11-06T01:08:03Z

x/ecocredit/client/query.go

+		Short: "Select the credit issuance batch info",
+		Long:  "Select the credit issuance batch info based on the bach_denom (ID)",


aaronc · 2020-11-06T01:08:20Z

x/ecocredit/client/query.go

+		Short: "Select the tradable and retired balance of the credit batch",
+		Long:  "Select the credit tradable and retired balance of the credit batch denom (ID) and account address",


aaronc · 2020-11-06T01:08:28Z

x/ecocredit/client/query.go

+		Short: "Select the maximum length of the fractional part of credits in the given batch",
+		Long:  "Select the maximum length of the fractional part of credits in the given batch. The precision tells what is the minimum unit of a credit.\nExample: a decimal number 12.345 has fractional part length equal 3. A precision=5 means that the minimum unit we can trade is 0.00001",


aaronc · 2020-11-06T01:08:41Z

x/ecocredit/client/query.go

+		Short: "Select the tradable and retired supply of the credit batch",
+		Long:  "Select the tradable and retired supply of the credit batch batch denom (ID)",


aaronc · 2020-11-09T21:44:28Z

x/ecocredit/module/module.go


 func (a AppModule) RegisterGRPCGatewayRoutes(sdkclient.Context, *runtime.ServeMux) {}

 func (a AppModule) RegisterInvariants(sdk.InvariantRegistry) {}

 func (a AppModule) RegisterServices(cfg module.Configurator) {
-	ecocredit.RegisterMsgServer(cfg.MsgServer(), a.Srv)
-	ecocredit.RegisterQueryServer(cfg.QueryServer(), a.Srv)
+	key := sdk.NewKVStoreKey(ecocredit.ModuleName)


The key cannot be created in RegisterServices. Then it won't get mounted with the multistore.

…obert/ecocredits-integrate

robert-zaremba added 2 commits November 5, 2020 16:20

Integrate ecocredits module

76469b1

use interfaces for Msg/Query services

79e555b

robert-zaremba marked this pull request as draft November 5, 2020 16:13

aaronc reviewed Nov 5, 2020

View reviewed changes

robert-zaremba added 3 commits November 5, 2020 19:37

simplify decimal parsing

56fd908

use protobuf server interfaces

8639f35

aaronc reviewed Nov 5, 2020

View reviewed changes

x/ecocredit/module/module.go Outdated Show resolved Hide resolved

robert-zaremba added 2 commits November 5, 2020 21:51

clean module.go

f8b2270

update app

b8a3d73

robert-zaremba requested review from anilcse, amaury1093 and blushi November 5, 2020 21:14

robert-zaremba added 3 commits November 5, 2020 23:45

Add Query CLI

6c56276

move client utility functions

dc04836

Adding MsgClient CLI: wip

9fbfcb0

robert-zaremba marked this pull request as ready for review November 5, 2020 23:36

aaronc reviewed Nov 6, 2020

View reviewed changes

robert-zaremba added 2 commits November 6, 2020 11:37

remove Server interface

9c255e7

add AppModuleBasic to enable interface types registration

228aec0

robert-zaremba mentioned this pull request Nov 6, 2020

ADR-031: elaborate consequences and encapsulation of module client-se… cosmos/cosmos-sdk#7839

Merged

8 tasks

aaronc reviewed Nov 9, 2020

View reviewed changes

aaronc added 3 commits November 10, 2020 11:14

fix tests

d5aa4f7

grammar check

51851c5

Merge branch 'master' of github.com:regen-network/regen-ledger into r…

18d09ea

…obert/ecocredits-integrate

aaronc approved these changes Nov 10, 2020

View reviewed changes

aaronc added 2 commits November 10, 2020 12:05

fix imports

0fcbfaa

fix lint

5bb66b2

aaronc merged commit 00ac858 into master Nov 10, 2020

ryanchristo deleted the robert/ecocredits-integrate branch July 2, 2022 22:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate ecocredits module #153

Integrate ecocredits module #153

robert-zaremba commented Nov 5, 2020 •

edited by zube bot

Loading

codecov bot commented Nov 5, 2020 •

edited

Loading

aaronc left a comment

aaronc Nov 5, 2020

robert-zaremba Nov 5, 2020

aaronc Nov 5, 2020

aaronc Nov 5, 2020

robert-zaremba Nov 5, 2020

robert-zaremba Nov 5, 2020

aaronc Nov 5, 2020

aaronc Nov 5, 2020

aaronc Nov 5, 2020

robert-zaremba Nov 5, 2020

aaronc commented Nov 5, 2020

aaronc Nov 5, 2020

robert-zaremba Nov 5, 2020

robert-zaremba Nov 5, 2020

aaronc Nov 5, 2020

aaronc Nov 5, 2020

robert-zaremba Nov 5, 2020

amaury1093 Nov 6, 2020 •

edited

Loading

robert-zaremba Nov 6, 2020

aaronc Nov 5, 2020

robert-zaremba commented Nov 5, 2020

aaronc left a comment

aaronc Nov 5, 2020

aaronc Nov 5, 2020

aaronc Nov 5, 2020

aaronc Nov 6, 2020

aaronc Nov 6, 2020

aaronc Nov 6, 2020

aaronc Nov 6, 2020

aaronc Nov 9, 2020

	Short: "Select a class info",
	Short: "Retrieve credit class info",

	Short: "Querying commands for the ecocredit module",
	Short: "Query commands for the ecocredit module",

		Short: "Select the credit issuance batch info",
		Long: "Select the credit issuance batch info based on the bach_denom (ID)",

		Short: "Select the tradable and retired balance of the credit batch",
		Long: "Select the credit tradable and retired balance of the credit batch denom (ID) and account address",

		Short: "Select the maximum length of the fractional part of credits in the given batch",
		Long: "Select the maximum length of the fractional part of credits in the given batch. The precision tells what is the minimum unit of a credit.\nExample: a decimal number 12.345 has fractional part length equal 3. A precision=5 means that the minimum unit we can trade is 0.00001",

		Short: "Select the tradable and retired supply of the credit batch",
		Long: "Select the tradable and retired supply of the credit batch batch denom (ID)",

Integrate ecocredits module #153

Integrate ecocredits module #153

Conversation

robert-zaremba commented Nov 5, 2020 • edited by zube bot Loading

codecov bot commented Nov 5, 2020 • edited Loading

Codecov Report

aaronc 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

aaronc commented Nov 5, 2020

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

amaury1093 Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robert-zaremba commented Nov 5, 2020

aaronc 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

robert-zaremba commented Nov 5, 2020 •

edited by zube bot

Loading

codecov bot commented Nov 5, 2020 •

edited

Loading

amaury1093 Nov 6, 2020 •

edited

Loading