Skip to content

rpcperms: add gRPC codes to errors #5633

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

rpcperms: add gRPC codes to errors #5633

wants to merge 1 commit into from

Conversation

DarthBenro008
Copy link

Eg:

  • Error Code starting with 01XX would be something related to wallet, maybe its not unlocked or already unlocked
  • Error Code starting with 10XX would be something related to the server itself, like its not still yet to startup fully

527A960B-F92D-40DD-A22F-C6771B49B75C

Signed-off-by: DarthBenro008 [email protected]

Pull Request Checklist

  • All changes are Go version 1.15 compliant
  • Your PR passes all CI checks. If a check cannot be passed for a justifiable reason, that reason must be stated in the commit message and PR description.
  • If this is your first time contributing, we recommend you read the Code Contribution Guidelines
  • For new code: Code is accompanied by tests which exercise both the positive and negative (error paths) conditions (if applicable)
  • For bug fixes: If possible, code is accompanied by new tests which trigger the bug being fixed to prevent regressions
  • Any new logging statements use an appropriate subsystem and logging level
  • For code and documentation: lines are wrapped at 80 characters (the tab character should be counted as 8 characters, not 4, as some IDEs do per default)
  • A description of your changes should be added to running the release notes for the milestone your change will land in.

@Roasbeef Roasbeef added this to the v0.14.0 milestone Aug 16, 2021
@carlaKC carlaKC self-requested a review August 17, 2021 10:06
carlaKC
carlaKC reviewed Aug 20, 2021
View reviewed changes
Copy link
Collaborator

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

Love the thought that went into these!

I think that it would be useful to declare the error codes as their own vars so that they can be imported by go code that's using the interceptor.

Eg:

waitingToStartCode = 1001

ErrWaitingToStart = status.Error(waitingToStartCode, "waiting to start, RPC services not "+
		"available")

@carlaKC
Copy link
Collaborator

carlaKC commented Aug 20, 2021

Also would be great to add a comment re the system you used to create these above the error code vars so that we remember it!

@DarthBenro008
feat: add gRPC codes to errors
5cab026 
- new gRPC Codes have been added for better error matching retaining the old text for string matching
- first digit indicates server related error
- second digit indicates wallet related error
- the third and fourth digit error number for the combination

Signed-off-by: DarthBenro008 <[email protected]>
@DarthBenro008
Copy link
Author

Hey @carlaKC , I have implemented the required changes and added few comments!

@DarthBenro008 DarthBenro008 requested a review from carlaKC August 20, 2021 14:33
carlaKC
carlaKC approved these changes Aug 24, 2021
View reviewed changes
Copy link
Collaborator

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

LGTM! Just needs an entry in the 0.14 release notes as well.

rpcperms/interceptor.go
@@ -5,6 +5,8 @@ import (
"fmt"
"sync"

"google.golang.org/grpc/status"

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can remove newline here

rpcperms/interceptor.go
var (
// ErrWaitingToStart is returned if LND is still wating to start,
// possibly blocked until elected as the leader.
ErrWaitingToStart = fmt.Errorf("waiting to start, RPC services not " +
ErrWaitingToStart = status.Error(ErrWaitingToStartStatusCode, "waiting to start, RPC services not "+
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: wrap at 80

@Roasbeef Roasbeef added the P2 should be fixed if one has time label Aug 31, 2021
@guggero guggero mentioned this pull request Aug 31, 2021
Closed
15 tasks
@guggero
Copy link
Collaborator

guggero commented Aug 31, 2021 •
edited
Loading

Thank you for the PR! I think the idea to use structured error codes is a great one!
I took your suggestion and described a counter proposal (that also takes into account the requirements of #5411) here: #5436 (comment)
That would lead to a more unified use of structured error codes and also a way to return error details in a structured way.

It means your PR would need some refactoring and the codes should probably be defined in the lnrpc package. What do you think, @DarthBenro008 ?

@DarthBenro008
Copy link
Author

Hi @guggero , I actually thought of something really similar while working on this PR. I'm really glad that we can have it implemented over a broader scope. I think I can work on refactoring the changes (refactoring as well as adding the new error codes in the lnrpc package) which would also address #5436 together.

I'd suggest opening a new issue mentioning both the PR's and relevant issues with the proposed methodology, and then I can open a new PR against the new issue. That would make it easier to track everything related to error codes in the long term.

If this is agreed upon, would be happy to open the issue 😃

@Roasbeef Roasbeef modified the milestones: v0.14.0, v0.15.0 Sep 27, 2021
@Roasbeef Roasbeef modified the milestones: v0.15.0, v0.16.0 Mar 7, 2022
@Roasbeef
Copy link
Member

Removing from the milestone for now. Can resurrect if the OP comes back.

@Roasbeef Roasbeef removed this from the v0.16.0 milestone Aug 23, 2022
@guggero guggero mentioned this pull request Aug 29, 2022
Open
@guggero guggero mentioned this pull request Aug 20, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlaKC carlaKC carlaKC approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
error codes gRPC P2 should be fixed if one has time
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

rpcperm: rpc state/status interceptor should return gRPC error codes
4 participants
@DarthBenro008 @carlaKC @guggero @Roasbeef