-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bianjie/lcd implementation #2118
Bianjie/lcd implementation #2118
Conversation
…to haoyang/lcd-implementation
Haoyang/lcd implementation
Refactor code according to lint result
Haoyang/lcd implementation
@jaekwon @jackzampolin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @HaoyangLiu, thanks so much for the work here. Could you change every reference of LCD
to Gaia-lite
?
client/context/context.go
Outdated
@@ -113,3 +116,15 @@ func (ctx CLIContext) WithUseLedger(useLedger bool) CLIContext { | |||
ctx.UseLedger = useLedger | |||
return ctx | |||
} | |||
|
|||
// WithCert - return a copy of the context with an updated Cert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Tendermint Lint to avoid verbose comments. You won't need to add the function name at the beginning of the comment with it
@@ -288,6 +288,23 @@ func (rs *rootMultiStore) Query(req abci.RequestQuery) abci.ResponseQuery { | |||
// trim the path and make the query | |||
req.Path = subpath | |||
res := queryable.Query(req) | |||
|
|||
// WARNING This should be consistent with query method in iavlstore.go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you open a separate issue for this warning ?
x/bank/client/rest/sendtx.go
Outdated
// RegisterSwaggerRoutes - Central function to define routes that get registered by the main application | ||
func RegisterSwaggerRoutes(routerGroup *gin.RouterGroup, ctx context.CLIContext, cdc *wire.Codec, kb keys.Keybase) { | ||
routerGroup.POST("/accounts/:address/send", sendRequestFn(cdc, ctx, kb)) | ||
routerGroup.POST("/create_transfer", createTransferTxForSignFn(cdc, ctx)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was changed to transfers
. See #1979
x/bank/client/rest/sendtx.go
Outdated
func RegisterSwaggerRoutes(routerGroup *gin.RouterGroup, ctx context.CLIContext, cdc *wire.Codec, kb keys.Keybase) { | ||
routerGroup.POST("/accounts/:address/send", sendRequestFn(cdc, ctx, kb)) | ||
routerGroup.POST("/create_transfer", createTransferTxForSignFn(cdc, ctx)) | ||
routerGroup.POST("/signed_transfer", composeAndBroadcastSignedTransferTxFn(cdc, ctx)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also suggest to change it to transfers
and add a boolean parameter called signed
in POST body. cc: @jackzampolin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a boolean parameter called signed in POST body
Could you clarify this? Do you mean merge /create_transfer and /signed_transfer into one rest api, such as /bank/transfers? I think this sound weird. How do you think about this? @jackzampolin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we are adding to the transfers
resource so the restful route here would be POST /bank/transfers
. It sounds like this route has two different behaviors:
- Create a transaction formatted for signing
- Create a transaction, sign and send this transaction.
I think the dominant behavior here would be to send a transaction and the transaction generation is more secondary. In the CLI we plan to handle this workflow (generate
, sign
,send
) by adding a --generate-only
flag to commands that normally do all 3.
The analogue here would be to add a query param (maybe ?generate=true
) that enables the generation behavior. What do you think @fedekunze @HaoyangLiu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Send query param in post request doesn't make sense semantically: explanation
I have another idea:
The below is the post body.
type sendBody struct {
Name string `json:"name"`
Password string `json:"password"`
Amount sdk.Coins `json:"amount"`
ChainID string `json:"chain_id"`
AccountNumber int64 `json:"account_number"`
Sequence int64 `json:"sequence"`
Gas int64 `json:"gas"`
Fee string `json:"fee"`
}
If both the two fields ("name" and "password") are not empty, then the transaction will be signed and broadcasted. If not, only generate a transaction and return the transaction bytes to user.
What do you think about this idea? @jackzampolin @fedekunze
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the explanation above. I like your idea more but I'd propose a slight change to it, since currently name
and password
are also used for testing automation purposes:
type sendBody struct {
Name string `json:"name"`
Password string `json:"password"`
Amount sdk.Coins `json:"amount"`
ChainID string `json:"chain_id"`
AccountNumber int64 `json:"account_number"`
Sequence int64 `json:"sequence"`
Gas int64 `json:"gas"`
Fee string `json:"fee"`
Signed bool `json:"signed"` // true by default
}
@jackzampolin thoughts ?
client/context/context.go
Outdated
|
||
// WithClientMgr - return a copy of the context with an updated ClientMgr | ||
func (ctx CLIContext) WithClientMgr(clientMgr *ClientManager) CLIContext { | ||
ctx.ClientMgr = clientMgr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change ctx.ClientMgr
to ctx.ClientManager
return nil, errors.New("missing node URIs") | ||
} | ||
|
||
func (mgr *ClientManager) getClient() rpcclient.Client { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(mgr *ClientManager)
––> (manager *ClientManager)
client/context/query.go
Outdated
@@ -277,6 +284,7 @@ func (ctx CLIContext) ensureBroadcastTx(txBytes []byte) error { | |||
return nil | |||
} | |||
|
|||
// nolint: gocyclo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactor the function and split it into smaller ones instead
client/keys/add.go
Outdated
@@ -236,6 +239,85 @@ func AddNewKeyRequestHandler(w http.ResponseWriter, r *http.Request) { | |||
w.Write(bz) | |||
} | |||
|
|||
// AddNewKeyRequest is the handler of adding new key in swagger rest server | |||
// nolint: gocyclo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here. Refactor and split
client/keys/add.go
Outdated
httputils.NewError(gtx, http.StatusBadRequest, err) | ||
return | ||
} | ||
err = json.Unmarshal(body, &m) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use cdc.UnmarshalJSON
for consistency
client/keys/delete.go
Outdated
var kb keys.Keybase | ||
var m DeleteKeyBody | ||
|
||
if err := gtx.BindJSON(&m); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use cdc.UnmarshalJSON
client/keys/list.go
Outdated
httputils.NewError(gtx, http.StatusInternalServerError, err) | ||
return | ||
} | ||
output, err := json.MarshalIndent(keysOutput, "", " ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cdc.MarshalJSON
httputils.NewError(gtx, http.StatusInternalServerError, err) | ||
return | ||
} | ||
output, err := json.MarshalIndent(keyOutput, "", " ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cdc.MarshalJSON
client/keys/update.go
Outdated
var kb keys.Keybase | ||
var m UpdateKeyBody | ||
|
||
if err := gtx.BindJSON(&m); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cdc.UnmarshalJSON
Another issue thats applicable here: #2113 |
Refactor lcd code according to code reviewers
Fix errors in test_cover and test_lint
I have refactor the code according to your feedback. Please take a look at the newest change. |
@HaoyangLiu random question... why did you selected |
// Which is much friendly for further development | ||
func ServeSwaggerCommand(cdc *wire.Codec) *cobra.Command { | ||
cmd := &cobra.Command{ | ||
Use: "rest-server-swagger", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a different name here? This is going to sit in the gaiacli
command, so maybe something like gaiacli lite serve
?
@fedekunze That is a long story. Previously, adrianbrink suggested me to create a PR to |
I have created another PR to develop, please move to the new PR: #2147 |
@HaoyangLiu why was this reopened ? |
Hi @fedekunze, I'm sorry for misleading. I reopen it just in case someone may miss these two PRs about lcd. Please move to new PR #2147. This one will be closed soon. |
Lcd spec location: https://github.com/cosmos/cosmos-sdk/tree/adrian/lcd_spec_final/docs/light
Main new feature:
Test report:
https://github.com/HaoyangLiu/cosmos-sdk/blob/haoyang/lcd-test/docs/light/Cosmos-LCD-Test-Report.md
Closes #525