Refactor/kos-go#255
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a new kosgo Go wrapper around kos_mobile: exported type aliases and constructors, a BigNumber wrapper with tests, signer wrappers and tests, wallet APIs (mnemonic/derivation/encryption, SignTransaction), extensive wallet tests, and an updated demo using kosgo APIs. ChangesGo SDK Wrapper for kos-mobile
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
packages/kos-go/signer_test.go (1)
17-33: ⚡ Quick winPrefer fail-fast error assertions in these tests.
For Line 17 and Line 29 flows, use
require.NoErrorso the test stops immediately when setup fails.Proposed patch
import ( "fmt" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) @@ xpub, err := GenerateXpub(MNEMONIC, passphrase, isMainnet, index) - - assert.Nil(t, err, "Failed to generate xpub") + require.NoError(t, err, "Failed to generate xpub") assert.Equal(t, 78, len(xpub), "Expected xpub length to be 78") @@ derivedXpub, err := DeriveXpub(MNEMONIC, passphrase, isMainnet, index, derivationPath) - - assert.Nil(t, err, "Failed to derive xpub") + require.NoError(t, err, "Failed to derive xpub") fmt.Printf("%v\n", []byte(derivedXpub)) assert.Equal(t, 78, len(derivedXpub), "Expected derived xpub length to be 78")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kos-go/signer_test.go` around lines 17 - 33, Replace the non-failing assertions in the tests with fail-fast error checks: change the calls that assert nil errors for GenerateXpub and DeriveXpub to use require.NoError (from testify/require) so the test aborts immediately on setup failures; update imports to include require where needed and keep the subsequent length assertions the same (references: GenerateXpub test and TestDeriveXpub using DeriveXpub).packages/kos-go/number_test.go (1)
165-205: ⚡ Quick winAdd explicit error-path tests for
Divand nil operands.Current tests validate many success paths, but they don’t assert wrapper behavior for divide-by-zero and nil inputs. Adding those cases will harden the API contract and catch panic regressions early.
Suggested additions
+func TestBigNumberErrorPaths(t *testing.T) { + a, err := NewBigNumber("10") + assert.Nil(t, err) + zero, err := NewBigNumber("0") + assert.Nil(t, err) + + _, err = Div(a, zero) + assert.Error(t, err, "division by zero should return error") + + _, err = Add(nil, a) + assert.Error(t, err, "nil lhs should return error") +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kos-go/number_test.go` around lines 165 - 205, The tests cover successful Div paths but miss error cases for divide-by-zero and nil operands; update TestBigNumberDivide (or add a new TestBigNumberDivideErrors) to assert Div returns an error (and not panic) when the divisor is zero (use NewBigNumber("0") and expect a non-nil error) and when either argument is nil (call Div(nil, b) and Div(a, nil) and assert errors), and ensure you check error types/messages consistently with Div's implementation to lock the API contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/kos-go/demo/main.go`:
- Around line 27-36: The demo currently ignores hex.DecodeString errors for
prevScript and proceeds after kosgo.SignTransaction failures; update main.go to
check and handle both errors immediately: validate the error returned by
hex.DecodeString("76a9...") (the prevScript variable) and log/exit if non-nil,
and when kosgo.SignTransaction(account, rawTx, options) returns an err, print
the error and terminate (or return) instead of continuing; locate usages of
prevScript and the call to kosgo.SignTransaction to add these early-failure
checks so the demo fails fast on decode or signing errors.
In `@packages/kos-go/number.go`:
- Around line 33-131: Guard all exported BigNumber APIs against nil
receiver/operands: check for nil *BigNumber and nil .inner before using .inner.
For methods like String (receiver b *BigNumber) return a safe zero value (e.g.,
"" or "nil") when b or b.inner is nil; for constructors/ops that return
(*BigNumber, error) such as Add, Sub, Mul, Div, Pow, Abs, Increment, Decrement
return (nil, fmt.Errorf("nil BigNumber operand")) when any input is nil; for
boolean predicates like IsEqual, IsGt, IsGte, IsLt, IsLte, IsZero, IsPositive,
IsNegative return false if any operand is nil. Apply these nil checks at the top
of each function (e.g., in Add, Sub, Mul, Div, Pow, Abs, Increment, Decrement,
String, IsEqual, IsGt, IsGte, IsLt, IsLte, IsZero, IsPositive, IsNegative) so no
.inner is accessed when operands are nil.
In `@packages/kos-go/signer.go`:
- Line 7: Add a correctly spelled public alias LdErrorInstanceError that points
to the same underlying type as the existing misspelled alias LdErrorIntanceError
so consumers can use the correct name while keeping the old alias for
compatibility; specifically, in signer.go add a new type alias declaration `type
LdErrorInstanceError = kos_mobile.LdErrorIntanceError` alongside the existing
`LdErrorIntanceError` declaration and leave the original alias untouched.
In `@packages/kos-go/wallet_test.go`:
- Line 239: The test currently ignores hex.DecodeString errors (e.g., the
prevScript assignment and other fixtures around the file) which can hide
malformed fixtures; update each hex.DecodeString call used in tests to capture
the returned error and call t.Fatalf with a clear message on decode failure
(e.g., replace `prevScript, _ := hex.DecodeString(...)` with capturing `err` and
doing `if err != nil { t.Fatalf("failed to decode prevScript fixture: %v", err)
}`), apply the same pattern for the other decode sites referenced (lines around
the other fixtures), ensuring you use the appropriate test variable (`t`) and
descriptive messages for each fixture name to fail fast.
In `@packages/kos-go/wallet.go`:
- Around line 62-70: The type switch in SignTransaction that assigns actualOpts
by dereferencing options (cases for *TransactionChainOptionsBtc,
*TransactionChainOptionsSubstrate, *TransactionChainOptionsEvm,
*TransactionChainOptionsCosmos) can panic when a typed-nil pointer is passed;
update each pointer case to check that v != nil before dereferencing and handle
the nil case (return an error or set a sensible default) instead of blindly
doing actualOpts = *v so dereferences are safe; keep references to the options
variable, the actualOpts target, and the specific types
(TransactionChainOptionsBtc, TransactionChainOptionsSubstrate,
TransactionChainOptionsEvm, TransactionChainOptionsCosmos) to locate and fix the
code.
---
Nitpick comments:
In `@packages/kos-go/number_test.go`:
- Around line 165-205: The tests cover successful Div paths but miss error cases
for divide-by-zero and nil operands; update TestBigNumberDivide (or add a new
TestBigNumberDivideErrors) to assert Div returns an error (and not panic) when
the divisor is zero (use NewBigNumber("0") and expect a non-nil error) and when
either argument is nil (call Div(nil, b) and Div(a, nil) and assert errors), and
ensure you check error types/messages consistently with Div's implementation to
lock the API contract.
In `@packages/kos-go/signer_test.go`:
- Around line 17-33: Replace the non-failing assertions in the tests with
fail-fast error checks: change the calls that assert nil errors for GenerateXpub
and DeriveXpub to use require.NoError (from testify/require) so the test aborts
immediately on setup failures; update imports to include require where needed
and keep the subsequent length assertions the same (references: GenerateXpub
test and TestDeriveXpub using DeriveXpub).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 452d8878-3b51-4373-a7a3-c4e195fb97ab
📒 Files selected for processing (8)
packages/kos-go/demo/main.gopackages/kos-go/models.gopackages/kos-go/number.gopackages/kos-go/number_test.gopackages/kos-go/signer.gopackages/kos-go/signer_test.gopackages/kos-go/wallet.gopackages/kos-go/wallet_test.go
ErnstBecker
approved these changes
May 12, 2026
grizante
approved these changes
May 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests