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

feat(cmd): use rpc client instead of http.Request #2521

Merged
merged 13 commits into from
Aug 7, 2023

Conversation

vgonkivs
Copy link
Member

@vgonkivs vgonkivs commented Jul 29, 2023

Overview

Ideas behind such rework are pretty simple:

  1. to simplify code.
    Me and @Wondertan love generic code but IMO the rpc is too complicated and hard to understand and maintain. Also, it contained a lot of hidden bugs that were found by @jcstein(bug: illegal base64 data at input byte during unmarshalling blob.Commitment #2500, blob.GetAll not returning correct base64 data, GetShare and GetSharesByNamespace also don't work #2503). I believe that extracting each module(or even a method) to its own command will simplify this package a lot.
  2. move from the raw http.Request.
    We've already had a rpc client but continue using http.Request. Also, the generic approach did not give us control over the returned value.

TODO: extract errors

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@vgonkivs vgonkivs self-assigned this Jul 29, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2023

Codecov Report

Merging #2521 (12b261d) into main (208bb86) will decrease coverage by 0.94%.
Report is 7 commits behind head on main.
The diff coverage is 8.88%.

@@            Coverage Diff             @@
##             main    #2521      +/-   ##
==========================================
- Coverage   52.64%   51.71%   -0.94%     
==========================================
  Files         156      157       +1     
  Lines        9990    10075      +85     
==========================================
- Hits         5259     5210      -49     
- Misses       4266     4409     +143     
+ Partials      465      456       -9     
Files Changed Coverage Δ
cmd/celestia/rpc.go 18.63% <3.70%> (+3.99%) ⬆️
cmd/celestia/blob.go 9.80% <9.80%> (ø)

... and 9 files with indirect coverage changes

@vgonkivs vgonkivs changed the title feat(rpc): lets keep blob separately feat(rpc): use rpc client instead of http.Request Jul 31, 2023
@vgonkivs vgonkivs added kind:misc Attached to miscellaneous PRs area:api Related to celestia-node API labels Jul 31, 2023
@vgonkivs vgonkivs marked this pull request as ready for review July 31, 2023 09:31
@jcstein
Copy link
Member

jcstein commented Jul 31, 2023

ack - LGTM

Screenshot 2023-07-31 at 11 50 40 (2)

@vgonkivs vgonkivs requested a review from jcstein July 31, 2023 12:33
@vgonkivs vgonkivs added kind:feat Attached to feature PRs and removed kind:misc Attached to miscellaneous PRs labels Jul 31, 2023
cmd/celestia/blob.go Outdated Show resolved Hide resolved
cmd/celestia/blob.go Outdated Show resolved Hide resolved
cmd/celestia/blob.go Outdated Show resolved Hide resolved
cmd/celestia/blob.go Outdated Show resolved Hide resolved
cmd/celestia/blob.go Outdated Show resolved Hide resolved
cmd/celestia/blob.go Outdated Show resolved Hide resolved
cmd/celestia/rpc.go Outdated Show resolved Hide resolved
cmd/celestia/rpc.go Outdated Show resolved Hide resolved
cmd/celestia/blob.go Outdated Show resolved Hide resolved
cmd/celestia/blob.go Outdated Show resolved Hide resolved
cmd/celestia/blob.go Outdated Show resolved Hide resolved
@Wondertan Wondertan changed the title feat(rpc): use rpc client instead of http.Request feat(cmd): use rpc client instead of http.Request Aug 1, 2023
cmd/celestia/blob.go Outdated Show resolved Hide resolved
cmd/celestia/blob.go Outdated Show resolved Hide resolved
cmd/celestia/blob.go Outdated Show resolved Hide resolved
cmd/celestia/blob.go Outdated Show resolved Hide resolved
cmd/celestia/blob.go Outdated Show resolved Hide resolved
renaynay
renaynay previously approved these changes Aug 4, 2023
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there's a lint issue, but otherwise I'm okay with it 🙏🏻 tested it myself (not the plaintext option yet, but that will be a really nice feature)

I'll make an issue for the commitment to also be returned for Submit.

@vgonkivs
Copy link
Member Author

vgonkivs commented Aug 4, 2023

I'll make an issue for the commitment to also be returned for Submit.

I've already implemented it @renaynay

@vgonkivs
Copy link
Member Author

vgonkivs commented Aug 4, 2023

FYI @jcstein, changes in Blob's commands:

  • they are now lower case(submit, get)
  • using - between words instead of camel case(get-all, get-proof)
  • submitting the data now returns not only the height but also the commitment of the Blob.
  • Added flag --plaintext that converts the Blob's data from base64 string(gm instead of Z20=). To enable this command you should add --plaintext=true in the end of the command

@vgonkivs
Copy link
Member Author

vgonkivs commented Aug 4, 2023

Снимок экрана 2023-08-04 в 14 18 42 Снимок экрана 2023-08-04 в 14 21 38

@vgonkivs vgonkivs merged commit a86ddca into celestiaorg:main Aug 7, 2023
13 of 15 checks passed
@jcstein
Copy link
Member

jcstein commented Aug 7, 2023

thanks for the context! a question on this before opening a PR to the docs:

are the rest of the methods still camelcase? this seems like it would get confusing as a user if we didn't change all methods.

i.e.

celestia rpc blob get-all [args...]
celestia rpc blob get [args...]
and then
celestia rpc state BalanceForAddress [args...]
celestia rpc p2p Info [args...]
celestia rpc header GetByHeight [ args...]
celestia rpc node Info
celestia rpc das SamplingStats

@jcstein
Copy link
Member

jcstein commented Aug 7, 2023

suggestion: allow --plaintext to set --plaintext=TRUE by default

@jcstein
Copy link
Member

jcstein commented Aug 7, 2023

i also think we need to set the API version to match the expected node release version cc @MSevey @adlerjohn

celestia rpc node Info

{
  "jsonrpc": "2.0",
  "result": {
    "type": 2,
    "api_version": "v0.2.1"
  },
  "id": 1
}

@vgonkivs
Copy link
Member Author

vgonkivs commented Aug 7, 2023

are the rest of the methods still camelcase? this seems like it would get confusing as a user if we didn't change all methods.

Yeah, they will be fixed in subsequent PRs

@jcstein
Copy link
Member

jcstein commented Aug 7, 2023

this plaintext flag is goated 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Related to celestia-node API kind:feat Attached to feature PRs
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants