-
Notifications
You must be signed in to change notification settings - Fork 2
refactor: moved ErrInternal from node repo and added ABCICode asserti… #181
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
Conversation
WalkthroughTests across multiple node modules were updated to use Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
go/node/provider/v1beta4/errors.go
Outdated
| errAttributes | ||
| errIncompatibleAttributes | ||
| errInvalidInfoWebsite | ||
| errInternal uint32 = 10 // Code must be 10 for backwards compatibility. Must stay explicit forever. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the most important part of this PR.
I decided to keep ABCI code for error from the node repo (it was 10), and left corresponding comment.
The gap is not a problem, it keeps backwards compatibility and requires no migrations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not a good idea as it breaks iota, as well as introduces possibility of duplicated id, just keep it in iota sequence, its fine if it gets a new id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I am keeping iota seq then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @go/node/provider/v1beta4/errors_test.go:
- Around line 69-74: The test and implementation currently use ABCI code 9 for
ErrInternal (errInternal defined as the 9th iota); the PR objectives require
preserving backward compatibility to ABCI code 10—align them by updating the
implementation and tests to use ABCI code 10: adjust the error constants in
errors.go (the errInternal definition and any iota offsets) so errInternal
becomes 10, update the exported v1beta4.ErrInternal mapping if present, and
change the test case in errors_test.go (the "internal" case expecting
expectedABCICode) to expect 10; also update any inline comments or PR docs that
reference the ABCI code to reflect the new value.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
go/node/provider/v1beta4/errors.gogo/node/provider/v1beta4/errors_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- go/node/provider/v1beta4/errors.go
🧰 Additional context used
🧬 Code graph analysis (1)
go/node/provider/v1beta4/errors_test.go (4)
go/node/provider/v1beta4/errors.go (9)
ErrInvalidProviderURI(22-22)ErrNotAbsProviderURI(25-25)ErrProviderNotFound(28-28)ErrProviderExists(31-31)ErrInvalidAddress(34-34)ErrAttributes(37-37)ErrIncompatibleAttributes(40-40)ErrInvalidInfoWebsite(43-43)ErrInternal(46-46)go/node/audit/v1/errors.go (2)
ErrProviderNotFound(16-16)ErrInvalidAddress(19-19)go/node/deployment/v1/errors.go (1)
ErrInternal(56-56)go/node/market/v1/errors.go (1)
ErrInternal(51-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: proto
- GitHub Check: go
- GitHub Check: coverage
- GitHub Check: test
🔇 Additional comments (3)
go/node/provider/v1beta4/errors_test.go (3)
6-6: LGTM!The
sdkerrorsimport alias is the conventional naming for the Cosmos SDK errors package and is required to access the*sdkerrors.Errortype and itsABCICode()method.
15-19: LGTM!The struct changes properly separate gRPC and ABCI code expectations, and using
*sdkerrors.Errorenables direct access toABCICode()without type assertions.
77-84: LGTM!The test execution correctly validates both aspects of the error codes. Using
status.FromError()to extract the gRPC status and directly callingABCICode()on the typed error is idiomatic for this SDK. The descriptive assertion messages will aid debugging if tests fail.
📝 Description
Testing
Tested in the
noderepo by running single node with chain-sdk changes.🔧 Purpose of the Change
📌 Related Issues
✅ Checklist
📎 Notes for Reviewers
I decided to keep ABCI code for error from the node repo (it was 10), and left corresponding comment.
The gap is not a problem, it keeps backwards compatibility and requires no migrations.