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(rpc): add share cmd #2664

Merged
merged 4 commits into from
Sep 13, 2023

Conversation

vgonkivs
Copy link
Member

@vgonkivs vgonkivs commented Sep 6, 2023

Overview

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 Sep 6, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 12, 2023

Codecov Report

❗ No coverage uploaded for pull request base (rpc_cmds_feature_branch@74fe698). Click here to learn what that means.
The diff coverage is n/a.

@@                    Coverage Diff                     @@
##             rpc_cmds_feature_branch    #2664   +/-   ##
==========================================================
  Coverage                           ?   49.19%           
==========================================================
  Files                              ?      163           
  Lines                              ?    11241           
  Branches                           ?        0           
==========================================================
  Hits                               ?     5530           
  Misses                             ?     5230           
  Partials                           ?      481           

}

var getSharesByNamespaceCmd = &cobra.Command{
Use: "get-shares-by-namespace [dah, namespace]",
Copy link
Member

Choose a reason for hiding this comment

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

get-by-namespace

}

var sharesAvailableCmd = &cobra.Command{
Use: "shares-available",
Copy link
Member

Choose a reason for hiding this comment

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

available

}

var getEDS = &cobra.Command{
Use: "get-eds [dah]",
Copy link
Member

Choose a reason for hiding this comment

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

Ryan be like (dah)

Copy link
Collaborator

Choose a reason for hiding this comment

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

да

Copy link
Collaborator

@distractedm1nd distractedm1nd left a comment

Choose a reason for hiding this comment

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

I agree with Hlib's review, will approve when those changes are made :) Wish there was an easy way to write unit tests for these

},
}

var probabilityOfAvailabilityCmd = &cobra.Command{
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know its not part of this PR but wanted to maybe start a discussion: This endpoint is dumb and should probably be removed entirely from the node, no?

Copy link
Member

@Wondertan Wondertan Sep 12, 2023

Choose a reason for hiding this comment

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

It's a feature request we had a while ago that we implemented quickly in the API. We still need it in some form or another. It should be merged with ShareAvailable call so that we don't just return binary answers, but a percentage of guaranteed local availability.

I suggest keeping the current cmd and merging it once we merge SharesAvailable API + shortening the current cmd name.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, we can merge it as it is and then remove it when it becomes redundant.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or I can return the percentage w SharesAvaliable(so available cmd will make 2 rpc call now)

Copy link
Member Author

Choose a reason for hiding this comment

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

Wish there was an easy way to write unit tests for these

Tested all cmds on arabica10

}

var probabilityOfAvailabilityCmd = &cobra.Command{
Use: "probability-of-availability",
Copy link
Member

Choose a reason for hiding this comment

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

We can call it availability. WDYT?

@Wondertan Wondertan merged commit 0794b5d into celestiaorg:rpc_cmds_feature_branch Sep 13, 2023
12 of 14 checks passed
vgonkivs added a commit to vgonkivs/celestia-node that referenced this pull request Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:rpc kind:feat Attached to feature PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants