modules/coin/rest: implemented doCreateRole #208

odeke-em · 2017-08-03T23:29:58Z

Implements doCreateRole

    $ curl -X POST http://localhost:8998/build/create_role --data '{"role":
    "abcdef", "sigs": 1, "signers": [{"addr":
    "4FF759D47C81754D8F553DCCAC8651D0AF74C7F9", "app": "role"}]}'

    HTTP/1.1 200 OK
    Content-Type: application/json
    Date: Fri, 04 Aug 2017 05:49:19 GMT
    Content-Length: 233
    
    {
      "type": "role/create",
      "data": {
        "role": "616263646566",
        "min_sigs": 1,
        "signers": [
          {
            "chain": "",
            "app": "role",
            "addr": "4FF759D47C81754D8F553DCCAC8651D0AF74C7F9"
          }
        ]
      }
    }

Updates #200

odeke-em · 2017-08-04T05:54:47Z

Alright, now CLI tests remaining.

ethanfrey · 2017-08-04T11:40:00Z

Cool, added functionality.

Two main points, I could add them to code, but easier here:

Please parse the input role name as hex. I will fix the cli to do the same (today?)
Please wrap the role with all the middleware (fee, chain, sigs) as with /build/send. All tx without this wrapping are kind of useless as they cannot be signed and will be rejected by the server upon post. (maybe you want to make a function to do this)?

odeke-em · 2017-08-04T21:02:37Z

Thanks for the notes @ethanfrey. In regards to:

I see you already implemented that, I've rebased
Unfortunately there isn't a fee associated with a role. I can only wrap it with: chain + signers.

odeke-em · 2017-08-04T21:59:56Z

Alright, I've updated the PR, PTAL.

odeke-em · 2017-08-08T05:50:37Z

After this PR is merged into unstable, there is another coming up on branch https://github.com/orijtech/basecoin/tree/query_role and that should conclude #200 or at least for the listed tasks.

ethanfrey

Looks good, but the create role handler belongs in modules/roles/rest

odeke-em · 2017-08-08T18:36:59Z

Thanks @ethanfrey, yeah that's a brain fart on my part, I'll move that code.

odeke-em · 2017-08-08T19:22:53Z

Updated @ethanfrey, PTAL!

ebuchman · 2017-08-10T22:44:08Z

modules/roles/rest/handlers.go

+	"github.com/tendermint/tmlibs/common"
+)
+
+type RoleInput struct {


Can we get a comment for the public struct?

Good plan, thanks for letting me know. I'll update this in a second.

ebuchman · 2017-08-10T22:44:46Z

modules/roles/rest/handlers.go

+
+// mux.Router registrars
+
+// RegisterQueryAccount is a mux.Router handler that exposes GET


This comment appears stale. Should be RegisterCreateRole and route is /build/create_role

Ah, thank you, updating it.

ebuchman · 2017-08-10T22:46:42Z

modules/roles/rest/handlers.go

+	tx = roles.NewCreateRoleTx(parsedRole, ri.MinimumSigners, ri.Signers)
+
+	// 3. ChainTx
+	tx = base.NewChainTx(commands.GetChainID(), 0, tx)


Not thrilled about calls to commands package all over. Think we need to revisit approach. It's certainly convenient and not that bad, just somewhat striking.

Yes I agree, perhaps we need a refactor in a few rounds. I think we had coupled all the functionality with the CLI(for which all these libraries had been built for) and in order to spin up the rest server (a quick job) we haven't been able to dissect and change it up.

ebuchman · 2017-08-10T22:49:06Z

cmd/baseserver/README.md

@@ -24,11 +24,17 @@ Route | Method | Completed | Description
 /sign|POST|✔️|Sign a transaction
 /tx|POST|✖️|Post a transaction to the blockchain
 /seeds/status|GET|✖️|Returns the information on the last seed
+/build/create_role|POST|✔️|Creates a role. Please note that the role MUST be valid hex for example instead of sending "role", send its hex encoded equivalent "726f6c65"


Not surprised to see this, but it doesn't make the best experience. Can we do something about it or is this a deep issue ?

So we changed this up in tendermint/basecoin@f2adf36 and the rational is that using hex makes sending content "more secure"/obfuscated as opposed to just sending a string, cc @ethanfrey.

Hmm ok. I'll wait to here from frey on this one. Thanks.

Yeah, we use hex identifiers everywhere and there was confusion writing tests with it not being hex. Then in a discussion, someone mentioned it was not good to use strings ("cosmos-devs") but rather force hex bytes which would generally be chosen with no human significance, and not leak information.

Seems easier and no need to use human-readable strings in the API, although one could build that on top, if so desired.

odeke-em · 2017-08-10T23:38:23Z

Thank you @ebuchman for the review, I've updated the feedback in tendermint/basecoin@d5b63ab, PTAL!

ebuchman · 2017-08-10T23:43:06Z

Great!

* Note: Role must be a hex string, as enforced in tests/rest/cli.sh ```shell $ curl -X POST http://localhost:8998/build/create_role --data \ '{ "role":"DEADBEEF", "seq": 1, "min_sigs": 1, "signers": [{ "addr": "4FF759D47C81754D8F553DCCAC8651D0AF74C7F9", "app": "role" }] }' ``` ```HTTP HTTP/1.1 200 OK Content-Type: application/json Date: Tue, 08 Aug 2017 19:15:13 GMT Content-Length: 387 { "type": "chain/tx", "data": { "chain_id": "test_chain_id", "expires_at": 0, "tx": { "type": "role/create", "data": { "role": "DEADBEEF", "min_sigs": 1, "signers": [ { "chain": "", "app": "role", "addr": "4FF759D47C81754D8F553DCCAC8651D0AF74C7F9" } ] } } } } ``` Updates #200

ethanfrey reviewed Aug 8, 2017

View reviewed changes

odeke-em mentioned this pull request Aug 8, 2017

Query role #220

Closed

ebuchman reviewed Aug 10, 2017

View reviewed changes

odeke-em mentioned this pull request Aug 16, 2017

document/standardize how we use hex tendermint/coding#11

Open

odeke-em added 3 commits August 18, 2017 22:32

modules/coin/rest: incorporated feedback from @ebuchman

dfae4cc

Fixed tests/cli/rest.sh for checking accounts

8a4e249

ethanfrey merged commit 7b3fde4 into cosmos:unstable Aug 18, 2017

odeke-em deleted the build_create branch August 29, 2019 02:39

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

Merge PR cosmos#208: Keyspace table

929a736

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

modules/coin/rest: implemented doCreateRole #208

modules/coin/rest: implemented doCreateRole #208

odeke-em commented Aug 3, 2017 •

edited

Loading

odeke-em commented Aug 4, 2017

ethanfrey commented Aug 4, 2017

odeke-em commented Aug 4, 2017

odeke-em commented Aug 4, 2017

odeke-em commented Aug 8, 2017

ethanfrey left a comment

odeke-em commented Aug 8, 2017

odeke-em commented Aug 8, 2017

ebuchman Aug 10, 2017

odeke-em Aug 10, 2017

ebuchman Aug 10, 2017

odeke-em Aug 10, 2017

ebuchman Aug 10, 2017

odeke-em Aug 10, 2017

ebuchman Aug 10, 2017

odeke-em Aug 10, 2017

ebuchman Aug 10, 2017

ethanfrey Aug 18, 2017

odeke-em commented Aug 10, 2017

ebuchman commented Aug 10, 2017


		// mux.Router registrars

		// RegisterQueryAccount is a mux.Router handler that exposes GET

modules/coin/rest: implemented doCreateRole #208

modules/coin/rest: implemented doCreateRole #208

Conversation

odeke-em commented Aug 3, 2017 • edited Loading

odeke-em commented Aug 4, 2017

ethanfrey commented Aug 4, 2017

odeke-em commented Aug 4, 2017

odeke-em commented Aug 4, 2017

odeke-em commented Aug 8, 2017

ethanfrey left a comment

Choose a reason for hiding this comment

odeke-em commented Aug 8, 2017

odeke-em commented Aug 8, 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

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

odeke-em commented Aug 10, 2017

ebuchman commented Aug 10, 2017

odeke-em commented Aug 3, 2017 •

edited

Loading