Skip to content
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

LCD design spec from Bianjie #1297

Closed
wants to merge 11 commits into from
Closed

Conversation

kidinamoto01
Copy link
Contributor

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG.md
  • Updated Gaia/Examples
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

In this spec, we present the design for latest LCD. Inputs are welcome!

@kidinamoto01 kidinamoto01 requested a review from ebuchman as a code owner June 19, 2018 08:44
@codecov
Copy link

codecov bot commented Jun 19, 2018

Codecov Report

Merging #1297 into adrian/lcd will increase coverage by 3.36%.
The diff coverage is n/a.

@@              Coverage Diff               @@
##           adrian/lcd    #1297      +/-   ##
==============================================
+ Coverage       65.26%   68.62%   +3.36%     
==============================================
  Files             102       86      -16     
  Lines            5519     4504    -1015     
==============================================
- Hits             3602     3091     -511     
+ Misses           1708     1221     -487     
+ Partials          209      192      -17

@gamarin2
Copy link
Contributor

Hey! Thank you, we need this spec :)

Initial feedback:

  • Can we separate the spec in two files. One for the LCD itself, and one for the API (endpoints).
  • The LCD file will be about running/configuring the rest server, describing the flags, etc. We should probably integrate this WIP that is about that: WIP: REST server docs #1139
  • The API file will be about describing all the available endpoints (this is mostly what you have done)

@adrianbrink
Copy link
Contributor

This looks great and is a fantastic starting point.

I agree with gamarin2 that it is nicer to split the document into two parts.

The first part is the design/specification for the LCD. It should focus on the high-level design and goals of the LCD. This document is what should allow anyone to implement the same functionality in another programming language.

The second part is the API document. Initially, the only module that it needs to describe is the bank module and how to query an account, send coins, ... . This part of the API can be called ICS20 (Inter-Chain-Standard 20). The AUTH module can be added as ICS19.

@kidinamoto01 Are you able to split it up according to this feedback?

It's important to remember that all querying from the LCD to the application state has to go through the query endpoint on ABCI.

I'm personally in-favour of removing the key management from the LCD. How do people feel about that?

@kidinamoto01
Copy link
Contributor Author

@gamarin2 @adrianbrink
Thanks for your feedbacks. Definitely gonna split the spec into 2 parts.

@HaoyangLiu
Copy link

@adrianbrink @gamarin2
Thanks for your feed back. I'm working with @kidinamoto01 to write the LCD spec.
I have a concern about removing key management module. This module is very important for implementing coin transfer. Without this module, our users have to build and sign transactions all by themselves. Besides, it is very convenient for us to build hot wallet with this module.
Maybe users will concern the security about their keys which will be imported to the key management module. Actually, the LCD node will be deployed in a local network and only serve for the users themselves. There is no way to access the LCD node from outside. So our users can totally trust their LCD nodes.
I'm personally in favor of keeping the key management. How do you feel about this?

@haifengxi
Copy link

haifengxi commented Jun 20, 2018

There might be a misunderstanding here. The spec doc Suyu committed was meant specifically for service providers, while there is a need to create a tech spec for other light client/wallet developers as well.

We should create a general LCD tech spec as @gamarin2 and @adrianbrink suggested @HaoyangLiu, and a separate "on-boarding guide" for service providers on top of the tech spec. @kidinamoto01

@kidinamoto01
Copy link
Contributor Author

kidinamoto01 commented Jun 20, 2018

@gamarin2 @adrianbrink
reorg the LCD spec, should be better now ^^
We have adopted ICS20 in API design, you could find the configuration for rest server.
I also added an overview.

@adrianbrink
Copy link
Contributor

This PR is superseded by #1314

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants