client/rest, modules/coin/rest: moved code around #197

odeke-em · 2017-07-31T22:11:45Z

After offline emails and a video call with @ethanfrey,
a goal was decided to move things around i.e:

Move /build/send and /query/account to modules/coin/rest

Due to that move, there is a lot of overlap between needed
code and utils so extracted common code to make
tendermint/tmlibs#33
so make sure to pull in that commit into your tmlibs tree.

odeke-em · 2017-07-31T22:12:00Z

/cc @ethanfrey

ethanfrey

Nice moving things...
More comments on cleaning it up, making it more configurable.

ethanfrey · 2017-08-01T20:46:21Z

client/rest/handlers.go

 	r.HandleFunc("/sign", doSign).Methods("POST")
-	r.HandleFunc("/tx", doPostTx).Methods("POST")


why did you (re)move /tx? It was good here.

/build/send and /query/account are good to have been moved.

That's def a mistake, thanks for reporting it.

ethanfrey · 2017-08-01T20:49:05Z

modules/coin/rest/handlers.go

+		return
+	}
+
+	if err := proofs.OutputProof(account, proof.BlockHeight()); err != nil {


OutputProof writes to stdout. Nothing to the request. No need to call it here.

You can just remove it and leave WriteSuccess how it is, but then it is missing the "height" context. Best to look at what OutputProof does to stdout and make a similar function that prints to a ResponseWriter.

So that function looks like this

and doesn't expose proof, I'll duplicate it then, just letting you know what's up with it.

Okay I've added an FoutputProof that does that what that fmt.Println business in OutputProof did.

ethanfrey · 2017-08-01T20:49:56Z

modules/coin/rest/handlers.go

+		return
+	}
+
+	tx := coin.NewSendOneTx(*si.From, *si.To, si.Amount)


This line is key here. And it involved parsing three variables, From, To, and Amount

ethanfrey · 2017-08-01T20:55:55Z

modules/coin/rest/handlers.go

+
+	tx := coin.NewSendOneTx(*si.From, *si.To, si.Amount)
+	// fees are optional
+	if si.Fees != nil && !si.Fees.IsZero() {


The lines from here down until success are all about wrapping the tx. Please pull this into one function, along with the parsing, and place that in client/rest as a helper. Why?

The next task to expose the create_role tx in /modules/roles/rest will duplicate that code.
-> this means extract into a common function

As soon as I ask you to support an optional assume_role middleware, you will have to change it everywhere. And if a third party wants to add a staking middleware, they will have to change what this common function does, ideally without forking
-> this means it should be configurable. Below is what I did for cli, it is a start, maybe you have a better idea.

https://github.com/tendermint/basecoin/blob/unstable/cmd/basecli/main.go#L55-L62
(it doesn't have to be so complicated with the wrappers, but at least some package exported variable like txcmd.Middleware to set what the wrappers are, even if I have to write it not compose it):

So I can extract that into a helper function, but I might have to keep that in modules/coin/rest because client/rest imports modules/coin/rest so that'd be an import cycle.

ethanfrey · 2017-08-01T20:59:06Z

client/rest/cmd.go

+	if err := ctx.RegisterHandlers(router); err != nil {
+		return err
+	}
+	if err := coinrest.RegisterHandlers(router); err != nil {


Rather than specifying the two packages where to register the handlers, please accept eg. a slice of RegisterHandler funcs, which can be set in main.go (at least indirectly). In a similar way how the cli allows one to register query functions.

https://github.com/tendermint/basecoin/blob/unstable/cmd/basecli/main.go#L43-L51

I think even ctx should be optional. A use case came up in the retreat where we want a baseserver that only allows queries. No need for key management or /tx or /send... That should be configurable without copying/forking large amounts of code.

ethanfrey · 2017-08-01T20:59:39Z

client/rest/handlers.go

@@ -42,39 +39,39 @@ func (k *Keys) GenerateKey(w http.ResponseWriter, r *http.Request) {
 	ckReq := &CreateKeyRequest{
 		Algo: k.algo,
 	}
-	if err := parseRequestJSON(r, ckReq); err != nil {
-		writeError(w, err)
+	if err := common.ParseAndValidateRequestJSON(r, ckReq); err != nil {


I agree this is good to move into tmlibs/common, just added comments on the PR, so we are happy with the implementation there.

ethanfrey · 2017-08-01T21:00:04Z

client/rest/cmd.go

+	coinrest "github.com/tendermint/basecoin/modules/coin/rest"
+)
+
+var ServeCmd = &cobra.Command{


Good to move this command here.

ethanfrey · 2017-08-01T21:00:33Z

client/rest/cmd.go

+var port int
+
+func init() {
+	ServeCmd.PersistentFlags().IntVar(&port, "port", 8998, "the port to run the server on")


Please use Int() not IntVar(). See next comment.

Also make "port" flag name a constant please.

Yeah Int() doesn't work alright, I think I mentioned this offline, it requires me then to invoke
ServeCmd.Parse(args) of which args aren't available until after we've entered the command .RunE and even then it isn't playing well with the flags cobra passes in []string{}. I spent too much on this that I figured I'd rather make functionality changes than be stuck trying to debug ServeCmd.

ethanfrey · 2017-08-01T21:02:50Z

client/rest/cmd.go

+		return err
+	}
+
+	addr := fmt.Sprintf(":%d", port)


If you used Int() above, you could use viper.GetInt(FlagPort) here.
This would allow us to set a BC_PORT variable (12-factor style) or add port = 12345 in the config file (typical unix style) to configure it and not just assume it comes from the command line.

Check out viper. It is pretty crazy. Maybe overkill but we decided on it for the cli here, so we can go all out.

Awesome, yeah I used viper.GetInt(flag) and it worked with cobra.PersistentFlags().Int(...), thanks for the advice!

ethanfrey · 2017-08-01T21:03:23Z

client/rest/handlers.go

 		return
 	}

-	writeSuccess(w, commit)
+	common.WriteSuccess(w, commit)
 }

 func doSign(w http.ResponseWriter, r *http.Request) {


This function is dead code now, right?

Nope, it is used in "/sign"

Sorry, I think I tagged the wrong function... scroll down to doSend. There are no references to it in the client/rest package.

You are right, thanks. Yeah, that was moved to modules/coin/rest/handlers.go.

ethanfrey · 2017-08-01T21:12:16Z

Oh, and if this depends on a special branch in tmlibs, please update the reference in glide.yaml and do glide update to reinitiate glide.lock and vendor dir. All dependencies must be set there to make it reproduceable.

odeke-em · 2017-08-02T06:45:39Z

I've addressed the other comments except for:

The middleware business

ethanfrey · 2017-08-02T12:14:54Z

client/commands/proofs/get.go

-func OutputProof(info interface{}, height uint64) error {
-	wrap := proof{height, info}
-	res, err := data.ToJSON(wrap)
+// FoutputProof writes the output of wrapping height and info


Very nice!

I like the generalization here that lets it work for http.ResponseWriter as well.

ethanfrey · 2017-08-02T12:15:13Z

client/rest/cmd.go

 }

 const defaultAlgo = "ed25519"

 func serve(cmd *cobra.Command, args []string) error {
+	port := viper.GetInt(envPortFlag)


ethanfrey · 2017-08-02T12:18:43Z

modules/coin/rest/handlers.go

@@ -63,17 +63,34 @@ func doQueryAccount(w http.ResponseWriter, r *http.Request) {
 		return
 	}

-	if err := proofs.OutputProof(account, proof.BlockHeight()); err != nil {
+	if err := proofs.FoutputProof(w, account, proof.BlockHeight()); err != nil {


ethanfrey · 2017-08-02T12:19:22Z

modules/coin/rest/handlers.go

-	common.WriteSuccess(w, account)
+}
+
+func PrepareSendTx(si *SendInput) basecoin.Tx {


Okay separate the validation/parse for the construction. Makes sense, also easier to test.

ethanfrey · 2017-08-02T12:21:29Z

modules/coin/rest/handlers.go

 }

 func doSend(w http.ResponseWriter, r *http.Request) {
 	defer r.Body.Close()
 	si := new(SendInput)
-	if err := common.ParseAndValidateRequestJSON(r, si); err != nil {


Okay, one question...

This ParseAndValidateRequestJSON does some auto-magic validation logic on the input.
Then below there is validate logic done manually.

I think we should pick one way or the other. Like use your validation checks, but then only ParseRequestJSON.

I think the check below is misleading if we think of it as "validation", it is a check on a variable passed in the query string, which is disjoint from the parsed struct so common.ParseAndValidateRequestJSON doesn't hurt.

ethanfrey · 2017-08-02T12:28:19Z

Okay, the middleware/handler registration in main can be a separate pr. I'll make an issue about this.

Things missing for a merge:

remove client/rest/doSend or convince me I am wrong
pick one validation method in modules/coin/rest/doSend and we'll use that for all rest calls.
update the glide.yaml / glide.lock file to the required version of tmlibs

I will look at the tmlibs pr now.

odeke-em · 2017-08-02T18:33:14Z

glide.lock

@@ -173,7 +173,7 @@ imports:
  - types
  - version
 - name: github.com/tendermint/tmlibs
-  version: 2f6f3e6aa70bb19b70a6e73210273fa127041070
+  version: 75372988e737a9f672c0e7f6308042620bd3e151


Please note that tmlibs should be used on branch develop with that hash

odeke-em · 2017-08-02T18:53:37Z

Done rebasing on origin/unstable.

@ethanfrey

After offline emails and a video call with @ethanfrey, a goal was decided to move things around i.e: - [X] Move /build/send and /query/account to modules/coin/rest Due to that move, there is a lot of overlap between needed code and utils so extracted common code to make tendermint/tmlibs#33 so make sure to pull in that commit into your tmlibs tree. After code review feedback: client/rest, modules/coin/rest: FoutputProof, PrepareSendTx helper * Extract OutputProof to FoutputProof helper that can be used in modules/coin/rest/handlers.go as proofs.FoutputProof * Revert r.HandleFunc("/tx", doPostTx).Methods("POST") which was erraneously deleted * Use function signatures from "tendermint/tmblibs/common"

odeke-em · 2017-08-03T18:03:27Z

Thanks for the review and merge @ethanfrey!

Problem: v0.47.x is outdated

ethanfrey suggested changes Aug 1, 2017

View reviewed changes

ethanfrey reviewed Aug 2, 2017

View reviewed changes

odeke-em commented Aug 2, 2017

View reviewed changes

odeke-em mentioned this pull request Aug 3, 2017

Baseserver send #203

Closed

ethanfrey approved these changes Aug 3, 2017

View reviewed changes

ethanfrey merged commit cdaee32 into cosmos:unstable Aug 3, 2017

ParthDesai pushed a commit to ChorusOne/cosmos-sdk that referenced this pull request Apr 19, 2021

Merge PR cosmos#197: Clarify scope of IBC

4e2f607

snyk-bot mentioned this pull request Jul 6, 2021

[Snyk] Upgrade vuepress-theme-cosmos from 1.0.156 to 1.0.182 terrorizer1980/cosmos-sdk#1

Closed

mmsqe added a commit to mmsqe/cosmos-sdk that referenced this pull request Feb 2, 2024

Merge pull request cosmos#197 from mmsqe/release/v0.47.x_sync_2

7b15830

Problem: v0.47.x is outdated

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client/rest, modules/coin/rest: moved code around #197

client/rest, modules/coin/rest: moved code around #197

odeke-em commented Jul 31, 2017

odeke-em commented Jul 31, 2017

ethanfrey left a comment

ethanfrey Aug 1, 2017

odeke-em Aug 2, 2017

ethanfrey Aug 1, 2017

odeke-em Aug 2, 2017

odeke-em Aug 2, 2017

ethanfrey Aug 1, 2017

ethanfrey Aug 1, 2017

odeke-em Aug 2, 2017

ethanfrey Aug 1, 2017

ethanfrey Aug 1, 2017

ethanfrey Aug 1, 2017

ethanfrey Aug 1, 2017

ethanfrey Aug 1, 2017

odeke-em Aug 2, 2017

ethanfrey Aug 1, 2017

odeke-em Aug 2, 2017

ethanfrey Aug 1, 2017

odeke-em Aug 2, 2017

ethanfrey Aug 2, 2017

odeke-em Aug 2, 2017

ethanfrey commented Aug 1, 2017

odeke-em commented Aug 2, 2017

ethanfrey Aug 2, 2017

ethanfrey Aug 2, 2017

ethanfrey Aug 2, 2017

ethanfrey Aug 2, 2017

ethanfrey Aug 2, 2017

odeke-em Aug 2, 2017

ethanfrey commented Aug 2, 2017

odeke-em Aug 2, 2017

odeke-em commented Aug 2, 2017

odeke-em commented Aug 3, 2017

		r.HandleFunc("/sign", doSign).Methods("POST")
		r.HandleFunc("/tx", doPostTx).Methods("POST")

client/rest, modules/coin/rest: moved code around #197

client/rest, modules/coin/rest: moved code around #197

Conversation

odeke-em commented Jul 31, 2017

odeke-em commented Jul 31, 2017

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

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 Aug 1, 2017

odeke-em commented Aug 2, 2017

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 Aug 2, 2017

Choose a reason for hiding this comment

odeke-em commented Aug 2, 2017

odeke-em commented Aug 3, 2017