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

refactor(rpc): move sanity check logic from rpc handlers to implementations. #1213

Merged
merged 2 commits into from
Oct 19, 2022

Conversation

HoytRen
Copy link
Contributor

@HoytRen HoytRen commented Oct 10, 2022

I see only 1 implementation. Tell me if here are more.

Close #998

Wondertan
Wondertan previously approved these changes Oct 10, 2022
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

LGTM, but defer the proper review to @renaynay and @distractedm1nd

state/core_access.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #1213 (37b1d8b) into main (18860df) will increase coverage by 0.16%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1213      +/-   ##
==========================================
+ Coverage   55.76%   55.92%   +0.16%     
==========================================
  Files         161      161              
  Lines        9650     9645       -5     
==========================================
+ Hits         5381     5394      +13     
+ Misses       3740     3720      -20     
- Partials      529      531       +2     
Impacted Files Coverage Δ
service/rpc/state.go 0.00% <ø> (ø)
state/core_access.go 12.34% <0.00%> (-0.85%) ⬇️
header/core/listener.go 52.83% <0.00%> (-5.67%) ⬇️
ipld/get_namespaced_shares.go 88.32% <0.00%> (-2.19%) ⬇️
fraud/pb/proof.pb.go 36.61% <0.00%> (+1.89%) ⬆️
share/discovery.go 35.41% <0.00%> (+3.12%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

distractedm1nd
distractedm1nd previously approved these changes Oct 12, 2022
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.

Looks great 🫡 thank you for the contribution!

@distractedm1nd distractedm1nd changed the title Move sanity check logic from rpc handlers to implementations. fix(rpc): move sanity check logic from rpc handlers to implementations. Oct 12, 2022
@distractedm1nd distractedm1nd changed the title fix(rpc): move sanity check logic from rpc handlers to implementations. refactor(rpc): move sanity check logic from rpc handlers to implementations. Oct 12, 2022
@HoytRen
Copy link
Contributor Author

HoytRen commented Oct 13, 2022

Rebased twice, and it still failed...

@Wondertan
Copy link
Member

Wondertan commented Oct 13, 2022

Yeah, we have some flakiness in tests

@HoytRen
Copy link
Contributor Author

HoytRen commented Oct 13, 2022

happy to see it passed :)

HoytRen and others added 2 commits October 19, 2022 11:09
Take Wondertan's suggestion.

Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>
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.

Thank you @HoytRen!

@renaynay renaynay merged commit 6340d45 into celestiaorg:main Oct 19, 2022
@HoytRen HoytRen deleted the #998 branch October 24, 2022 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rpc: Move sanity check logic from rpc handlers to implementations
5 participants