test: improve SDK test coverage from 3% to 54%#183
Conversation
Add comprehensive unit tests across all pure SDK packages with table-driven tests, testify assertions, and edge case coverage. Packages covered: - pkg/common/decimals: 100% (13 math functions) - pkg/common: 94% (hex, hash, base58, presentation) - pkg/common/numeric: 93% (50+ Dec operations) - pkg/keys/hd: 96% (BIP44 HD wallet with BIP32 test vectors) - pkg/address: 92% (address conversions and round-trips) - pkg/keys: 44% (key generation, encoding, mnemonic derivation) - pkg/keystore: 71% (encrypt/decrypt, account CRUD, sign/verify) - pkg/contract: 100% (ABI JSON parsing) - pkg/abi: 100% (ABI encoding/decoding, event parsing) - pkg/account: 85% (import/export/removal with temp dirs) - pkg/mnemonic: 100% (BIP39 generation and validation) - pkg/store: 74% (local account management) Bug fixes discovered during testing: - Fix Pow(x, 0) returning x instead of 1 - Fix EncodeCheck mutating caller's input slice - Fix nil dereference in ExportPrivateKey/ExportKeystore - Fix DerivePrivateKeyForPath empty segment handling - Add legacy ABI stateMutability resolution (constant/payable) Also: - Exclude pkg/proto and cmd from coverage reporting - Add Codecov integration to CI workflow - Refactor store path helpers to support test isolation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds many new unit tests across packages, tightens account/keystore path handling and input validation, hardens key/seed handling with memory zeroing, updates CI to shuffle tests and use Codecov, fixes base58 mutation, adds common.ZeroBytes, and introduces a broken change in pkg/common/decimals/decimals.go that causes a compile error. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
🚥 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)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/common/decimals/decimals.go (1)
5-11:⚠️ Potential issue | 🟡 MinorHandle negative exponents or document the constraint.
The
for range esyntax is fine—go.modalready specifies Go 1.24.0, which supports it.However, negative exponents aren't handled: if
e < 0, the loop executes zero times and returns 1 instead of the correct value1/a^|e|. The function acceptsint64(which allows negative values) but has no validation, guard, or documentation. Either:
- Add validation to reject negative exponents and return an error, or
- Implement the correct behavior (recursively handle negatives like
pkg/common/numeric/numeric.godoes), or- Add a comment documenting that only non-negative exponents are supported.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/decimals/decimals.go` around lines 5 - 11, Pow currently ignores negative exponents (e < 0) and returns 1; update Pow to handle negatives consistently (or explicitly reject them). Implement the correct behavior by detecting e < 0, computing p := Pow(a, -e) and returning the reciprocal (Quot(NewFloat(1), p)) so Pow(a, -n) == 1 / Pow(a, n); keep using NewFloat and Mul for positive exponent paths and ensure you use big.Float division for the reciprocal. Reference the Pow function and helper constructors NewFloat and Mul when making the change.pkg/account/import.go (1)
104-124:⚠️ Potential issue | 🔴 CriticalPrevent path traversal via account
namebefore buildingnewPath.
nameis user-controlled and is joined directly into the destination path (Line 123). Values like../...or path separators can write outsidestore.DefaultLocation().🔒 Suggested fix
if name == "" { name = generateName() + "-imported" for store.DoesNamedAccountExist(name) { name = generateName() + "-imported" } } else if store.DoesNamedAccountExist(name) { return "", fmt.Errorf("account %s already exists", name) } + if name == "." || name == ".." || strings.ContainsAny(name, `/\`) || filepath.IsAbs(name) { + return "", fmt.Errorf("invalid account name %q", name) + } key, err := keystore.DecryptKey(keyJSON, passphrase) if err != nil { return "", err } @@ // create home dir if it doesn't exist store.InitConfigDir() - newPath := filepath.Join(store.DefaultLocation(), name, filepath.Base(keyPath)) + baseDir := filepath.Clean(store.DefaultLocation()) + newPath := filepath.Join(baseDir, name, filepath.Base(keyPath)) + rel, err := filepath.Rel(baseDir, newPath) + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return "", fmt.Errorf("invalid keystore destination for account %q", name) + } err = writeToFile(newPath, string(keyJSON))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/account/import.go` around lines 104 - 124, The account import builds newPath using the user-controlled variable name (in pkg/account/import.go) which allows path traversal; before creating newPath (and before calling writeToFile) validate and sanitize name (e.g., reject or strip path separators and "..", use filepath.Base or filepath.Clean on name) and then verify the final destination is contained inside store.DefaultLocation() (e.g., join DefaultLocation() and cleaned name and ensure the cleaned absolute path has the DefaultLocation() as its prefix) so malicious values cannot escape the keystore directory; apply this validation both for user-supplied name and the generated-name branch.
🧹 Nitpick comments (3)
pkg/keys/mnemonic.go (1)
17-18: Consider zeroing sensitive key material after use.The
seed(64 bytes) andmastersecret contain sensitive cryptographic material. Per secure key handling practices, these should be zeroed when no longer needed to minimize exposure in memory.seed := bip39.NewSeed(mnemonic, passphrase) master, ch := hd.ComputeMastersFromSeed(seed, []byte("Bitcoin seed")) + defer func() { + for i := range seed { + seed[i] = 0 + } + for i := range master { + master[i] = 0 + } + }()This is a broader pattern across the codebase but worth considering for key derivation paths.
As per coding guidelines: "Cryptographic key management with HD wallets (BIP39/BIP44). Pay close attention to security: key material handling, constant-time comparisons, and proper zeroing of sensitive data."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/keys/mnemonic.go` around lines 17 - 18, Zero the sensitive byte slices produced during BIP39/BIP32 derivation: after calling bip39.NewSeed (seed) and hd.ComputeMastersFromSeed (master), explicitly overwrite the contents of seed and master with zeros as soon as they are no longer needed (for example via a small wipe loop) and ensure the wipe runs even on early returns (use defer immediately after creation). Reference the variables seed and master and the functions bip39.NewSeed and hd.ComputeMastersFromSeed in mnemonic.go; place the zeroing before returning or handing off non-secret derived values so no sensitive material remains in memory.pkg/keystore/keystore_test.go (1)
401-443: Harden timed-unlock tests against clock/scheduler flakiness.The short fixed sleeps at Line 408, Line 411, Line 425, and Line 440 can be brittle in slower CI/race runs. Prefer eventual polling assertions with wider timing margins.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/keystore/keystore_test.go` around lines 401 - 443, The tests use short fixed time.Sleep calls in the timed-unlock cases which can flake; replace those deterministic sleeps with eventual/polling assertions (e.g. require.Eventually or a small loop that retries SignHash until success/failure) around the calls to ks.TimedUnlock / ks.Unlock and the subsequent ks.SignHash checks so the test waits up to a generous timeout for the expected state; update the "timed unlock can extend previous timeout", "timed unlock can shorten previous timeout", and "indefinite unlock is not altered by timed unlock" cases to poll SignHash (or use eventuallySignHashFails helper) instead of fixed sleeps after invoking TimedUnlock/Unlock on the ks created by newTestKeyStore and accounts created by NewAccount, using randomHash as before.pkg/keystore/key_test.go (1)
143-148: Add a companionstoreNewKeytest with key-file verification enabled.Line 147 disables key-file verification, so this path cannot catch secure storage regressions (permissions/verification behavior). Keep this fast case, but add another case using default verification settings.
As per coding guidelines
pkg/keystore/**: "Encrypted keystore implementation. Review for secure storage practices, proper encryption, and safe key derivation."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/keystore/key_test.go` around lines 143 - 148, Add a companion test to the existing storeNewKey test that exercises key-file verification by creating a second keyStorePassphrase instance without skipKeyFileVerification set (or explicitly false) and using the same keysDirPath, scryptN/scryptP (LightScryptN/LightScryptP) to create and load a key so permission/verification behavior is validated; ensure the new test mirrors the fast case steps (create store, call storeNewKey, then attempt to read/verify the key file) but uses the default verification settings to catch regressions in secure storage and file-permissions handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/abi/abi_extended_test.go`:
- Around line 261-285: The tests and conversion helpers currently suppress
parsing errors (using _, _ or ignoring the ok return from big.Int.SetString),
causing invalid/overflow numeric inputs to be coerced to zero; update the
conversion functions convertToInt, convertSmallIntSlice and any
big.Int.SetString usages to check and return descriptive errors when
strconv.ParseInt/ParseUint or SetString fail (propagate errors up to the caller
rather than returning zero), and update the tests (the cases in
TestConvertToInt_InvalidStrings and the other test blocks around lines ~737–759)
to expect an error (assert error) or a failed conversion instead of
assert.NotPanics/expecting success so invalid and overflow inputs fail fast.
Ensure error messages include the type name (from eABI.NewType/ty) and the
offending input for easier debugging.
In `@pkg/account/account_test.go`:
- Around line 27-30: Replace the raw literal testPrivateKey constant with a
deterministic, non-secret generator to avoid secret-scan failures: remove the
hardcoded const testPrivateKey and either make it a var populated at init or add
a helper (e.g., getTestPrivateKey or initTestKeys) that derives a 32-byte value
deterministically (for example SHA-256 of a fixed test-only seed like
"pkg/account test key") and hex-encodes that result for use in tests; update
references in account_test.go to use the new helper/var (symbols: testPrivateKey
-> getTestPrivateKey or init-populated testPrivateKey) so tests remain
deterministic but no real private-key literal exists in the repo.
In `@pkg/account/removal.go`:
- Around line 19-22: The call to os.RemoveAll(accountDir) currently ignores any
error and always returns nil; change this to capture the error (err :=
os.RemoveAll(accountDir)), and if non-nil return a wrapped or contextualized
error (e.g., include the account name and accountDir) instead of nil so callers
see failures from account removal; update the surrounding function return path
where accountDir, store.DefaultLocation() and os.RemoveAll are used to propagate
this error.
In `@pkg/keys/keys_test.go`:
- Around line 91-144: The "valid 32 bytes" case in TestGetPrivateKeyFromBytes
currently uses an all-zero 32-byte slice which is not a valid secp256k1 private
key; update that test's input to use a non-zero 32-byte value (e.g., create a
32-byte slice and set at least one byte to non-zero, similar to the "valid 32
bytes with non-zero value" subtest) so keys.GetPrivateKeyFromBytes (which calls
btcec.PrivKeyFromBytes) returns a valid key and the test passes.
In `@pkg/store/local.go`:
- Around line 19-20: The call to homedir.Dir() currently ignores errors (uDir, _
:= homedir.Dir()), which can yield an empty uDir and cause filepath.Join to
return a relative path; update the code to check the error returned by
homedir.Dir() and handle it explicitly (return or propagate an error) instead of
proceeding with an empty uDir. Specifically, in the function that builds the
config path (where uDir, DefaultConfigDirName, homedir.Dir(), and filepath.Join
are used) validate err from homedir.Dir() and return a clear error or wrap it so
keystore initialization never falls back to the current working directory; make
the same change in pkg/keys/keys.go at the similar homedir.Dir() usage.
---
Outside diff comments:
In `@pkg/account/import.go`:
- Around line 104-124: The account import builds newPath using the
user-controlled variable name (in pkg/account/import.go) which allows path
traversal; before creating newPath (and before calling writeToFile) validate and
sanitize name (e.g., reject or strip path separators and "..", use filepath.Base
or filepath.Clean on name) and then verify the final destination is contained
inside store.DefaultLocation() (e.g., join DefaultLocation() and cleaned name
and ensure the cleaned absolute path has the DefaultLocation() as its prefix) so
malicious values cannot escape the keystore directory; apply this validation
both for user-supplied name and the generated-name branch.
In `@pkg/common/decimals/decimals.go`:
- Around line 5-11: Pow currently ignores negative exponents (e < 0) and returns
1; update Pow to handle negatives consistently (or explicitly reject them).
Implement the correct behavior by detecting e < 0, computing p := Pow(a, -e) and
returning the reciprocal (Quot(NewFloat(1), p)) so Pow(a, -n) == 1 / Pow(a, n);
keep using NewFloat and Mul for positive exponent paths and ensure you use
big.Float division for the reciprocal. Reference the Pow function and helper
constructors NewFloat and Mul when making the change.
---
Nitpick comments:
In `@pkg/keys/mnemonic.go`:
- Around line 17-18: Zero the sensitive byte slices produced during BIP39/BIP32
derivation: after calling bip39.NewSeed (seed) and hd.ComputeMastersFromSeed
(master), explicitly overwrite the contents of seed and master with zeros as
soon as they are no longer needed (for example via a small wipe loop) and ensure
the wipe runs even on early returns (use defer immediately after creation).
Reference the variables seed and master and the functions bip39.NewSeed and
hd.ComputeMastersFromSeed in mnemonic.go; place the zeroing before returning or
handing off non-secret derived values so no sensitive material remains in
memory.
In `@pkg/keystore/key_test.go`:
- Around line 143-148: Add a companion test to the existing storeNewKey test
that exercises key-file verification by creating a second keyStorePassphrase
instance without skipKeyFileVerification set (or explicitly false) and using the
same keysDirPath, scryptN/scryptP (LightScryptN/LightScryptP) to create and load
a key so permission/verification behavior is validated; ensure the new test
mirrors the fast case steps (create store, call storeNewKey, then attempt to
read/verify the key file) but uses the default verification settings to catch
regressions in secure storage and file-permissions handling.
In `@pkg/keystore/keystore_test.go`:
- Around line 401-443: The tests use short fixed time.Sleep calls in the
timed-unlock cases which can flake; replace those deterministic sleeps with
eventual/polling assertions (e.g. require.Eventually or a small loop that
retries SignHash until success/failure) around the calls to ks.TimedUnlock /
ks.Unlock and the subsequent ks.SignHash checks so the test waits up to a
generous timeout for the expected state; update the "timed unlock can extend
previous timeout", "timed unlock can shorten previous timeout", and "indefinite
unlock is not altered by timed unlock" cases to poll SignHash (or use
eventuallySignHashFails helper) instead of fixed sleeps after invoking
TimedUnlock/Unlock on the ks created by newTestKeyStore and accounts created by
NewAccount, using randomHash as before.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
.github/workflows/test.yamlMakefilepkg/abi/abi_extended_test.gopkg/account/account_test.gopkg/account/export.gopkg/account/import.gopkg/account/removal.gopkg/address/address_test.gopkg/common/base58.gopkg/common/base58_test.gopkg/common/decimals/decimals.gopkg/common/decimals/decimals_test.gopkg/common/hash_test.gopkg/common/hexutils_test.gopkg/common/numeric/numeric_test.gopkg/common/presentation_test.gopkg/contract/abi.gopkg/contract/abi_test.gopkg/keys/hd/hdwallet.gopkg/keys/hd/hdwallet_test.gopkg/keys/keys_test.gopkg/keys/mnemonic.gopkg/keystore/account_test.gopkg/keystore/key_test.gopkg/keystore/keystore_test.gopkg/mnemonic/mnemonic_test.gopkg/store/local.gopkg/store/local_test.go
fbsobreira
left a comment
There was a problem hiding this comment.
Thanks for the thorough review! Here's what we've addressed:
Fixed in this round:
pkg/account/account_test.go: Added// gitleaks:allowannotation to the test private key constantpkg/account/removal.go: Now propagatesos.RemoveAllerror instead of silently ignoring itpkg/keys/keys_test.go: Changed "valid 32 bytes" test case to use a non-zero value (secp256k1 requires non-zero private keys)pkg/account/import.go: Added path traversal prevention — rejects account names containing/,\,., or..pkg/keys/mnemonic.go: Addeddeferblocks to zero sensitiveseedandmasterkey material after use
Tracked as GitHub issues (out of PR scope):
- #184 — ABI error propagation in
convertToInt/convertSmallIntSlice(requires broader signature changes) - #185 — Negative exponent handling in
Powfunction (behavior change affecting callers)
Declined with rationale:
pkg/store/local.gohomedir.Dir()error handling: This is pre-existing code not introduced by this PR. The concern is valid but belongs in a separate fixpkg/keystore/keystore_test.gotimed-unlock flakiness: The timing tests use conservative margins (200ms sleep for 100ms timeout) and haven't shown flakiness in CI. Switching to polling would add complexity without clear benefitpkg/keystore/key_test.gocompanion verification test: The current test already validates key storage; adding a second variant with default verification settings provides minimal additional coverage
fbsobreira
left a comment
There was a problem hiding this comment.
Thanks for the thorough review! Here is what we addressed:
Fixed in this round:
pkg/account/account_test.go: Added// gitleaks:allowannotation to the test private key constantpkg/account/removal.go: Now propagatesos.RemoveAllerror instead of silently ignoring itpkg/keys/keys_test.go: Changed "valid 32 bytes" test case to use a non-zero value (secp256k1 requires non-zero private keys)pkg/account/import.go: Added path traversal prevention - rejects account names containing/,\,., or..pkg/keys/mnemonic.go: Addeddeferblocks to zero sensitiveseedandmasterkey material after use
Tracked as GitHub issues (out of PR scope):
- #184 - ABI error propagation in
convertToInt/convertSmallIntSlice(requires broader signature changes) - #185 - Negative exponent handling in
Powfunction (behavior change affecting callers)
Declined with rationale:
pkg/store/local.gohomedir.Dir()error handling: Pre-existing code not introduced by this PRpkg/keystore/keystore_test.gotimed-unlock flakiness: Conservative timing margins, no CI flakiness observedpkg/keystore/key_test.gocompanion verification test: Minimal additional value over existing coverage
- Add gitleaks:allow annotation to test private key constant - Propagate os.RemoveAll error in RemoveAccount instead of ignoring it - Use non-zero bytes for valid secp256k1 private key test case - Add path traversal prevention for account names in ImportKeyStore - Zero sensitive seed and master key material after use in mnemonic derivation
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/keys/keys_test.go (1)
360-362: Mark this fixed test key to avoid gitleaks false positives.Line 361 is a known test vector, but scanners may treat it as a real secret. Add an inline
gitleaks:allowannotation (or repository allowlist) to keep CI alerts high-signal.Suggested fix
{ name: "known mnemonic-derived key", - keyHex: "b5a4cea271ff424d7c31dc12a3e43e401df7a40d7412a15750f3f0b6b5449a28", + keyHex: "b5a4cea271ff424d7c31dc12a3e43e401df7a40d7412a15750f3f0b6b5449a28", // gitleaks:allow },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/keys/keys_test.go` around lines 360 - 362, The test vector in the table entry with name "known mnemonic-derived key" contains a hardcoded keyHex test value that triggers gitleaks; add an inline annotation to mark it as intentionally allowed (e.g., a trailing comment like // gitleaks:allow or the repository's preferred allowlist annotation) immediately next to the keyHex literal in the test case so scanners skip it and CI won't flag a false positive; target the test case struct entry containing keyHex "b5a4..." in keys_test.go (the test vector for "known mnemonic-derived key") when adding the annotation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/keys/mnemonic.go`:
- Around line 23-33: The chain code variable ch returned by
hd.ComputeMastersFromSeed is sensitive and remains in memory after use; after
calling hd.DerivePrivateKeyForPath (which uses master and ch) wipe ch in the
same deferred cleanup used for master and seed so the chain code bytes are
zeroed (e.g., add a loop setting ch[i]=0 in the existing defer), ensuring you
perform the wipe after DerivePrivateKeyForPath returns and before any early
returns in the function where hd.ComputeMastersFromSeed and
hd.DerivePrivateKeyForPath are used.
---
Nitpick comments:
In `@pkg/keys/keys_test.go`:
- Around line 360-362: The test vector in the table entry with name "known
mnemonic-derived key" contains a hardcoded keyHex test value that triggers
gitleaks; add an inline annotation to mark it as intentionally allowed (e.g., a
trailing comment like // gitleaks:allow or the repository's preferred allowlist
annotation) immediately next to the keyHex literal in the test case so scanners
skip it and CI won't flag a false positive; target the test case struct entry
containing keyHex "b5a4..." in keys_test.go (the test vector for "known
mnemonic-derived key") when adding the annotation.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/account/account_test.gopkg/account/import.gopkg/account/removal.gopkg/keys/keys_test.gopkg/keys/mnemonic.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/account/removal.go
- pkg/account/import.go
- pkg/account/account_test.go
- Add common.ZeroBytes helper for wiping sensitive byte slices - Use ZeroBytes to zero seed, master, and chain code in mnemonic derivation - Add gitleaks:allow annotations to all test key hex literals
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/keys/mnemonic.go`:
- Around line 14-16: The current check only rejects negative indices ("if index
< 0") but allows very large positive indices which can be truncated when later
converted to uint32; update the validation around the "index" variable to also
reject values greater than math.MaxUint32 (or explicitly > 0xFFFFFFFF) and
return an error instead of proceeding to path derivation, so downstream uint32
conversions cannot wrap/truncate; modify the same function containing the "if
index < 0" check to enforce "index >= 0 && uint64(index) <= math.MaxUint32" (and
return a descriptive error) before any derivation logic runs.
- Around line 23-33: The derived private-key byte slice named private returned
by hd.DerivePrivateKeyForPath must be zeroed before returning; after calling
btcec.PrivKeyFromBytes(private[:]) (in the code using hd.DerivePrivateKeyForPath
and btcec.PrivKeyFromBytes) overwrite every byte of private with zeros (best via
a simple for i := range private { private[i] = 0 } or a small helper like
zeroBytes) and ensure this happens even on early returns (use a defer
immediately after successful derivation to clear private). Do not change the
call order (derive -> create btcec key -> zero), and keep the zeroing local to
the private slice.
- Reject index > math.MaxUint32 to prevent uint32 truncation - Zero derived private key bytes after btcec conversion
Summary
pkg/proto/andcmd/from coverage reportingPer-Package Coverage
pkg/common/decimalspkg/abipkg/contractpkg/mnemonicpkg/keys/hdpkg/commonpkg/common/numericpkg/addresspkg/accountpkg/storepkg/keystorepkg/keysBug Fixes
Pow(x, 0)returnedxinstead of1EncodeCheckmutated caller's input slice via appendExportPrivateKey/ExportKeystorenil dereference when address not in keystoreDerivePrivateKeyForPathpanicked on empty path segmentsFromMnemonicSeedAndPassphraseignored derivation errorsresolveStateadded for legacy ABI compatibility (constant/payable flags)Test Plan
make test— all tests pass with race detectionmake lint— no new lint errors in test filesgo tool coverSummary by CodeRabbit
Tests
Bug Fixes
New Features
Refactor
Chores