R4R: REST Client Unsafe/Safe Endpoints

[alexanderbez]

• Remove client/keys/codec.go in favor already valid codec in codec/codec.go
• Move common functionality and types from client/keys/* to client/keys/common
  • This allows client/keys to use a CLIContext! (cc @jleni @alessio)
• Update all appropriate REST handlers to handle unsafe routes

closes: #3560

/cc @cosmos/cosmos-ui

---

• Targeted PR against correct branch (see CONTRIBUTING.md)

• Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

• Wrote tests

• Updated relevant documentation (docs/)

• Added entries in PENDING.md with issue #

• rereviewed Files changed in the github PR explorer

---

For Admin Use:

• Added appropriate labels to PR (ex. wip, ready-for-review, docs)
• Reviewers Assigned
• Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

[codecov]

Codecov Report

Merging #3640 into develop will increase coverage by 0.03%.
The diff coverage is 29.5%.

@@             Coverage Diff             @@
##           develop    #3640      +/-   ##
===========================================
+ Coverage    60.79%   60.82%   +0.03%     
===========================================
  Files          189      187       -2     
  Lines        14170    14092      -78     
===========================================
- Hits          8615     8572      -43     
+ Misses        5011     4980      -31     
+ Partials       544      540       -4

[faboweb]

What are the implications for this? What would change exactly? Can you give me an example of what we would need to change if this is done?

[alexanderbez]

@faboweb no routes will changes. Simply any routes that actually sign txs and all of the /keys/* will need to have the rest server started with --allow-unsafe.

[alexanderbez]

I initially went with a ClientRouter wrapper struct that contained a mux router and an unsafe bool, but found the route registration ugly as there was a if/else switch in every module's registration handler

The following solution leaves it up to the actual handler and relies on the context. Also, not super clean, but I feel like it is more DRY compared to the previous solution.

[alexanderbez]

In order to wrap this up, we need to pass a CLIContext to the keys REST handlers. This causes import cycles. I would like to fix this as part of this PR, but I also don't want to step on the toes of #3580. So I say this is blocked until that is merged and then I can wrap this up.

/cc @jackzampolin @alessio

[faboweb]

I get it now. Nice change.

[alexanderbez]

@faboweb thanks! I believe this will leave Voyager largely, if not entirely, unaffected as from what @fedekunze tells me, Voyager performs the generate only => sign offline => broadcast flow, which is the ideal flow 👌

[melekes]

🍷

[alexanderbez]

Ok, so with advent of #3641, this should still be merged but only exist for keybase routes.

[alexanderbez]

closing in favor of #3674