Create app #23

orkunkl · 2019-07-29T10:40:00Z

Closes #4
This is basically a copy-paste from the tutorial. If you think if there is an unnecessary chain decorator, let me know

ethanfrey

Nice start. Made some comments to clean up app.
Clearly docs need to exist to explain this. Thanks for figuring this out without proper docs.

ethanfrey · 2019-07-29T11:44:27Z

app/app.go

+// Router returns a default router
+func Router(authFn x.Authenticator) *app.Router {
+	r := app.NewRouter()
+	// TODO implement orderbook router


Replace this TODO with the multisig and cash router at least.

ethanfrey · 2019-07-29T11:45:21Z

app/app.go

+		sigs.NewDecorator(),
+		multisig.NewDecorator(authFn),
+		utils.NewSavepoint().OnDeliver(),
+		msgfee.NewFeeDecorator(),


msgfee can probably be removed from base app.
And the cash.StaticFeeDecorator can be used above (this is the simplest case when the product fee doesn't depend on the output from the Handler)

ethanfrey · 2019-07-29T11:45:58Z

app/app.go

+// allowing access to "/auth", "/contracts" and "/"
+func QueryRouter() weave.QueryRouter {
+	r := weave.NewQueryRouter()
+	r.RegisterAll(


I would add cash here, as this is basic to most apps.
And msgfee if you don't remove the decorator above.

I thought we should keep weave-starter-kit simple as possible. So deleted msgfee

ethanfrey · 2019-07-29T11:47:04Z

app/codec.proto

+  repeated bytes multisig = 4;
+  // msg is a sum type over all allowed messages on this chain.
+  oneof sum {
+    cash.SendMsg cash_send_msg = 51;


You can also add the create/update multisig msgs here

ethanfrey · 2019-07-29T11:47:30Z

app/codec.proto

+    // space here to allow many more....
+
+    custom.CreateCustomStateIndexedMsg custom_create_state_indexed_msg = 100;
+    custom.CreateCustomStateMsg create_custom_state_msg = 101;


If you add these, please update the Router and QueryHandler set in the app to include these

I still need to implement handler and buckets. Lets keep this PR on until I finish and merge them

ethanfrey · 2019-07-29T11:50:52Z

app/init.go

+// DecorateApp adds initializers and Logger to an Application
+func DecorateApp(application app.BaseApp, logger log.Logger) app.BaseApp {
+	application.WithInit(app.ChainInitializers(
+		&migration.Initializer{},


Please update this to be in-line with the router/query/etc.

There are a few places that need to be kept in sync - good to add this to the docs.

When adding a package:

Stack if it has a Decorator

Router if it has a Handler

QueryRouter if it has any Buckets

DecorateApp/init if it has any Initializer.

Best to write down a list of all modules to use first, then make them all the same. As discussed above, I think the minimal set is:

cash

sigs

multisig

currency

migration (as a runtime dependency of above modules)

validators is a nice to have to run a PoA testnet and good to have as a demo.

--

msgfee is a bit more complex and requires more complex Fee handling. I would not use this for default app.

ethanfrey · 2019-07-29T11:54:02Z

x/custom/codec.proto

@@ -2,7 +2,7 @@ syntax = "proto3";

 package custom;

-import "github.com/iov-one/weave/codec.proto";
+import "codec.proto";


This didn't work the other way?
I actually thought we wanted short paths for references in the same repo, and full for outside.

But if needed for protoc, then do it like this.

I tried to make it work as it was but no chance 😞 The only thing worked is to make x/codec imports refer to weave codec without the prefix.

Okay, then we just document that. Maybe someone (maybe me?) has time to look into it. Otherwise we just explain how to work with this.

I would appreciate it if somebody takes a look at it. I spent quite a time on the issue and this is the solution I come up with ¯_(ツ)_/¯

ethanfrey · 2019-07-29T11:59:04Z

About protoc... these are key lines in prototool.yaml.

 includes:
    - .
    - /usr/include
    - spec
    - spec/github.com/iov-one/weave

. means that import "x/custom/codec.go" will work inside this app (recommended to avoid GOPATH issues)

spec means that we can import "github.com/iov-one/weave/coins/codec.proto" and it will use the protobuf file from our spec dir (which must be kept up to date with the weave repo).

spec/github.com/iov-one/weave is needed as if we import eg. github.com/iov-one/weave/x/cash/codec.proto, it imports coins/codec.proto using relative imports to it's project. When we then import this via spec, the relative import doesn't work without this second line.

That is - customapp imports modules from same repo with short path. customapp imports weave protobuf from spec with full path. weave protobuf inside spec imports other weave protobuf with short path.

Does this make sense? It was how I got it to work once (how it still does). But I really should have documented this better. Sorry.

orkunkl · 2019-08-06T14:10:36Z

Blocked by #31

husio

If you want to make this crips, please update the comments to follow https://github.com/golang/go/wiki/CodeReviewComments#comment-sentences

husio · 2019-08-12T09:22:34Z

app/app.go

+	)
+}
+
+/* custom app uses below weave modules apart from x/custom


I am not sure why this comment is here. I think if at all, it should be part of general application documentation or package level comment. This place seems quite random for this listing.

Nice advise 👍 I really enjoy writing go because only language itself covers and enforces structuring and documenting your code.

husio · 2019-08-12T09:25:09Z

app/app.go

+	// Expand the path fully
+	path, err := filepath.Abs(dbPath)
+	if err != nil {
+		return nil, fmt.Errorf("Invalid Database Name: %s", path)


Error string should not be capitalized https://github.com/golang/go/wiki/CodeReviewComments#error-strings
Maybe it is worth using weave/errors package to produce a better error instance?

husio · 2019-08-12T09:27:27Z

app/app.go

+	ctx := context.Background()
+	kv, err := CommitKVStore(dbPath)
+	if err != nil {
+		return app.BaseApp{}, err


I think it is better to wrap error with additional information. Such context information is useful during debugging. For example:

return app.BaseApp{}, errors.Wrap(err, "cannot create database instance")

husio · 2019-08-12T09:28:31Z

app/codec.proto

+// Tx contains the message
+message Tx {
+  // fee info, autogenerates GetFees()
+  cash.FeeInfo cash_fees = 1;


Is the above comment true? This attribute will generate GetCashFees method. 🤔

https://github.com/iov-one/weave/blob/master/x/cash/codec.pb.go#L220-L225 Auto-generation does not cover the concept though.

It is not necessary to add comments for autogeneration. Just remove it to not create confusion with all the other fields where it is not added.

husio · 2019-08-12T09:29:05Z

app/codec.proto

+  repeated sigs.StdSignature sigs_signatures = 2;
+  // ID of a multisig contract.
+  repeated bytes multisig = 4;
+  // msg is a sum type over all allowed messages on this chain.


This attribute is called sum not msg 🚓

husio · 2019-08-12T09:30:10Z

app/init.go

+		dict  map[string]interface{}
+		array []interface{}
+	)
+	collectorAddr, err := hex.DecodeString("3b11c732b8fc1f09beb34031302fe2ab347c5c14")


I think it is worth adding a comment what collector address is (to whom it belongs to?) and why it is hardcoded here.

Nice point. I will take bnsd/init as an example because mycoind/initseems too much outdated

husio · 2019-08-12T09:35:18Z

app/init.go

+// You can give coins to this address and return the recovery
+// phrase to the user to access them.
+func GenerateCoinKey() (weave.Address, string, error) {
+	// TODO implement BIP39 recovery phrases


I think a TODO does not belong in a good codebase. Either implement it or leave a good comment explaining why below functionality works in this certain way and what was the reason for choosing this implementation.

Leaving TODO like this will give an impression that the code is not finished and lacking, therefore cannot be trusted. No one will address this comment as well.

husio · 2019-08-12T09:37:05Z

app/tx.go

+// make sure tx fulfills all interfaces
+var _ weave.Tx = (*Tx)(nil)
+
+// GetMsg switches over all types defined in the protobuf file


I think a much better comment explaining the functionality would be

GetMsg returns a single message instance that is represented by this transaction.

alpe

Only had a very brief look:

Please run go fmt $(go list ./... | grep -v "/vendor/")
make protoc fails on my box

<input>:1:1:Import "x/custom/codec.proto" was not found.
app/codec.proto:29:5:"custom.CreateTimedStateMsg" is not defined.
app/codec.proto:30:5:"custom.CreateStateMsg" is not defined.

make test fails on my box:

app/app.go:23:2: unknown import path "github.com/iov-one/weave-starter-kit/x/custom": import lookup disabled by -mod=readonly

alpe · 2019-08-12T13:34:26Z

app/codec.proto

+// Tx contains the message
+message Tx {
+  // fee info, autogenerates GetFees()
+  cash.FeeInfo cash_fees = 1;


It is not necessary to add comments for autogeneration. Just remove it to not create confusion with all the other fields where it is not added.

orkunkl · 2019-08-12T14:47:33Z

@alpe really weird you are having errors on make protoc. I don't have any. Do you have any guess why you could be getting errors?

orkunkl · 2019-08-13T14:17:46Z

Waiting for response from gogo/protobuf#601

orkunkl · 2019-08-15T10:04:30Z

I have fixed harmless protoc bug in iov-one/weave#953. Changes will be applied when upgraded to weave v0.21.0

orkunkl requested review from husio, alpe, ruseinov and ethanfrey July 29, 2019 10:40

ethanfrey reviewed Jul 29, 2019

View reviewed changes

This was referenced Jul 29, 2019

Document protool and protoc usage iov-one/docs#47

Closed

Document weave decorators iov-one/docs#48

Open

orkunkl requested a review from ethanfrey July 29, 2019 14:58

orkunkl added 17 commits August 9, 2019 15:39

Run go mod tidy

0bd1308

Fix protool import path

628a4b0

Fix codec import paths

079f196

Create init.go

375e416

Add tx.go

ed76ea2

Add app codec

fe33654

Compile codec.proto

2fada2c

Add app.go

7162b5f

Rename application to prjct

38b7b27

Fix tx codec numbers and add required modules

d653f18

Add required modules to router, query router and organize

2c8fc5a

Add decorators and organize code

07cf469

Fix codec and compile

db72d8a

Run make mod

1bb4725

Add x/custom codec messages to app/codec

2a87aa7

Add custom package migration schema

ccd74cb

Register custom package to Router and QueryRouter

665df47

orkunkl force-pushed the create-app branch from b9d1001 to 665df47 Compare August 9, 2019 13:18

husio approved these changes Aug 12, 2019

View reviewed changes

Remove app modules comment from app.go

9215fd6

orkunkl added 5 commits August 12, 2019 13:29

Creage doc.go and explain app package

58a5dfc

Remove fmt

de4c130

Add database error message detail when CommitKVStore fails

8b085f4

Improve commenting on codec

a107ccb

Improve app/codec/sum comment

15fefee

alpe reviewed Aug 12, 2019

View reviewed changes

orkunkl added 4 commits August 12, 2019 17:35

Make requested changes on init.go

f7fa711

Add detail to conf/migration/admin

36f8075

Improve tx.go GetMsg() commenting

89c9df3

Remove weave/errors from init.go

431f2a7

orkunkl added 2 commits August 12, 2019 17:47

Compile app codec.proto

53e0a21

Run go fmt on all code

23dd47f

orkunkl mentioned this pull request Aug 13, 2019

Document prototool usage iov-one/docs#85

Merged

orkunkl mentioned this pull request Aug 14, 2019

Fix make protoc #37

Closed

orkunkl merged commit 482e603 into master Aug 15, 2019

orkunkl deleted the create-app branch August 15, 2019 10:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create app #23

Create app #23

orkunkl commented Jul 29, 2019

ethanfrey left a comment

ethanfrey Jul 29, 2019

ethanfrey Jul 29, 2019

ethanfrey Jul 29, 2019

orkunkl Jul 29, 2019

ethanfrey Jul 29, 2019

ethanfrey Jul 29, 2019

orkunkl Jul 29, 2019

ethanfrey Jul 29, 2019

ethanfrey Jul 29, 2019

orkunkl Jul 29, 2019

ethanfrey Jul 29, 2019

orkunkl Jul 29, 2019

ethanfrey commented Jul 29, 2019

orkunkl commented Aug 6, 2019

husio left a comment

husio Aug 12, 2019

orkunkl Aug 12, 2019

husio Aug 12, 2019

husio Aug 12, 2019

husio Aug 12, 2019

orkunkl Aug 12, 2019

alpe Aug 12, 2019

husio Aug 12, 2019

husio Aug 12, 2019

orkunkl Aug 12, 2019 •

edited

Loading

husio Aug 12, 2019

husio Aug 12, 2019

alpe left a comment

alpe Aug 12, 2019

orkunkl commented Aug 12, 2019 •

edited

Loading

orkunkl commented Aug 13, 2019

orkunkl commented Aug 15, 2019

Create app #23

Create app #23

Conversation

orkunkl commented Jul 29, 2019

ethanfrey 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

ethanfrey commented Jul 29, 2019

orkunkl commented Aug 6, 2019

husio 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

orkunkl Aug 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orkunkl commented Aug 12, 2019 • edited Loading

orkunkl commented Aug 13, 2019

orkunkl commented Aug 15, 2019

orkunkl Aug 12, 2019 •

edited

Loading

orkunkl commented Aug 12, 2019 •

edited

Loading