-
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
client/rest, cmd/baseserver: started a basecoin REST client #190
Conversation
/cc @ethanfrey PTAL at the first draft of the server to get a base committed. After this first review I'll go back and then update /build/send and init and then we can have an MVP. Thank you! |
cmd/baseserver/main.go
Outdated
port := 8998 | ||
router := mux.NewRouter() | ||
ctx := rest.Context{ | ||
Keys: rest.New(keysManager, ""), |
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.
You can safely set the default here as "ed25519".
We use it like 97% of the time... secp256k1 is mainly used to make sure other algorithms work
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.
Ah I see, thanks. I'll update it.
|
||
// TODO: Decide whether to use $HOME/.basecli for compatibility | ||
// or just use $HOME/.baseserver? | ||
cmd := cli.PrepareMainCmd(srvCli, "BC", os.ExpandEnv("$HOME/.basecli")) |
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.
Nice job figuring out how to use our system here and just the essentials.
r.HandleFunc("/keys/{name}", k.DeleteKey).Methods("DELETE") | ||
} | ||
|
||
type Context struct { |
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.
Looks nice and clean
// Note: Not using the `validator:""` tags here because SendInput has | ||
// many fields so it would be nice to figure out all the invalid | ||
// inputs and report them back to the caller, in one shot. | ||
type SendInput struct { |
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.
nice struct here.
reuse of existing structs and pointers for checking missing fields.
client/rest/handlers.go
Outdated
|
||
var tx basecoin.Tx | ||
coins := []coin.Coin{*si.Fees} | ||
if !si.Multi { // Just doing one transaction |
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.
nice call with NewSendOneTx, but multi is a different meaning. It is to control wrapping the last with auth.OneSig or auth.MultiSig.
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.
Ooh I see, thanks for letting me know. I'll do another iteration and work on fixing it up.
} | ||
|
||
tx := sr.Tx | ||
if err := SignTx(sr.Name, sr.Password, tx); 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.
Looks good.
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.
Thank you!
client/rest/handlers.go
Outdated
} | ||
|
||
var tx basecoin.Tx | ||
coins := []coin.Coin{*si.Fees} |
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.
You need si.Amount for the amount to send
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.
Roger that, thanks I'll update that.
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.
Actually @ethanfrey, si.Fees is a Coin so it contains the amount and denomination, so we are covered here.
client/rest/handlers.go
Outdated
tx = coin.NewSendTx(in, out) | ||
} | ||
|
||
commit, err := PostTx(tx) |
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.
PostTx call should go into another endpoint POST /tx
, which accepts a previously signed tx and just posts.
/build
constructs the json, but doesn't sign or post.
Something (roughly) like:
tx = NewFeeTx(tx, *si.Fee)
tx = NewNonceTx(tx, si.Sequence)
tx = NewChainTx(tx, commands.GetChainID())
if multi {
tx = auth.NewMulti(tx).Wrap()
} else {
tx = auth.NewSig(tx).Wrap()
}
package names missing, some functions wrong, but basically this idea.
Looks good. Very nice to jump into this codebase and get it working. I left some comments, mainly on |
Oh, also, please add a |
```shell $ go get -u -v github.com/tendermint/basecoin/cmd/baseserver $ baseserver init $ baseserver serve ``` A server that can be ran by default on port 8998 otherwise one can specify the port using flag `--port` like this: ```shell $ baseserver serve --port 9999 ``` to serve it on port 9999, accessible at http://localhost:9999 Implemented: - [X] /keys POST -- generate a new key - [X] /keys GET -- list all keys - [X] /keys/{name} DELETE-- delete a named key - [X] /keys/{name} GET -- get a named key - [X] /keys/{name} POST, PUT -- update a named key - [X] /sign POST -- sign a transaction - [X] /build/send POST -- send money from one actor to another. However, still needs testing and verification of output - [X] /tx POST -- post a transaction to the blockchain. However, still needs testing and verification of output This base code to get the handlers starters was adapted from: * https://github.com/tendermint/go-crypto/blob/master/keys/server * https://github.com/tendermint/basecoin/blob/unstable/client/commands/proxy/root.go Updates #186
Done, thanks, I added the |
@ethanfrey, thank you for the first round of review! I've updated the PR PTAL. |
Nice job. Looks much better. There are a few things with build still. I'll make a PR on the PR to clear it up. |
Oh, I think there is one thing missing... You should be able to base very heavily (almost copy-paste) from Other than that, the MVP looks finished |
Allows us to query account balance from the blockchain for example: /query/account/sigs:BDADF167E6CF2CDF2D621E590FF1FED2787A40E0
@ethanfrey I've updated the PR with /query/account/{signature}, PTAL, thanks! |
func doAccountQuery(w http.ResponseWriter, r *http.Request) { | ||
query := mux.Vars(r) | ||
signature := query["signature"] | ||
address, errResp := extractAddress(signature) |
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.
This works, but no need to run extractAddress... ParseActor does that for you.
Not wrong, just unnecessary
return | ||
} | ||
|
||
if err := proofs.OutputProof(account, proof.BlockHeight()); 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.
Output proof writes to stdout, no need to call it here, the writeSuccess takes it's place.
writeError(w, err) | ||
return | ||
} | ||
writeSuccess(w, account) |
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.
If you look at OutputProof, it just adds an envelope around the data to add height info to the json. You can do it here before writeSuccess.
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.
Very nice. Just added two comments for cleanup.
I'll merge this in and test it with the frontend guys. Just those comments to note when you do a cleanup.
A server that can be ran on port 8998
Implemented:
needed
This base code to get the handlers starters was adapted from:
Updates #186