Add ValidateAddress function to check onchain and offchain addresses#136
Open
tiero wants to merge 2 commits into
Open
Add ValidateAddress function to check onchain and offchain addresses#136tiero wants to merge 2 commits into
tiero wants to merge 2 commits into
Conversation
Adds an exported ValidateAddress(address string) function that infers the network from the address prefix and returns whether it is a valid Bitcoin onchain address or Ark offchain address, without requiring a configured client. https://claude.ai/code/session_0151wY6W7ib1h3HWof32Xz7q
WalkthroughAdded a new exported function Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller
participant Val as ValidateAddress
participant BTC as btcutil.DecodeAddress
participant ARK as arklib.DecodeAddressV0
Caller->>Val: ValidateAddress(address)
Val->>BTC: try decode (mainnet/testnet3/signet/regtest/Mutiny)
alt BTC decode succeeds
BTC-->>Val: decoded (onchain)
Val-->>Caller: (isOnchain=true, isOffchain=false, nil)
else BTC decode fails for all nets
Val->>ARK: DecodeAddressV0(address)
alt ARK decode succeeds
ARK-->>Val: decoded (offchain)
Val-->>Caller: (isOnchain=false, isOffchain=true, nil)
else ARK decode fails
ARK-->>Val: error
Val-->>Caller: (false, false, error)
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
utils.go (1)
151-160: Prefer a single source of truth for supported Bitcoin networks.Lines 151-157 duplicate network mapping that already exists via
toBitcoinNetwork. This can drift as networks evolve.♻️ Proposed refactor
func ValidateAddress(address string) (isOnchain bool, isOffchain bool, err error) { - knownNetworks := []*chaincfg.Params{ - &chaincfg.MainNetParams, - &chaincfg.TestNet3Params, - &chaincfg.SigNetParams, - &chaincfg.RegressionNetParams, - &arklib.MutinyNetSigNetParams, - } - for _, net := range knownNetworks { - if _, e := btcutil.DecodeAddress(address, net); e == nil { + knownNetworks := []arklib.Network{ + arklib.Bitcoin, + arklib.BitcoinTestNet, + arklib.BitcoinSigNet, + arklib.BitcoinRegTest, + arklib.BitcoinMutinyNet, + } + for _, net := range knownNetworks { + params := toBitcoinNetwork(net) + if _, e := btcutil.DecodeAddress(address, ¶ms); e == nil { return true, false, nil } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils.go` around lines 151 - 160, The hardcoded knownNetworks slice duplicates logic from toBitcoinNetwork and risks drifting; replace the inline knownNetworks usage in the address validation loop with the canonical network set returned or referenced by toBitcoinNetwork (e.g., iterate over the networks/mapping provided by toBitcoinNetwork instead of creating &chaincfg.MainNetParams, &chaincfg.TestNet3Params, &chaincfg.SigNetParams, &chaincfg.RegressionNetParams, &arklib.MutinyNetSigNetParams), then call btcutil.DecodeAddress(address, net) as before and return the same values; remove the duplicated slice so the code uses the single source of truth in toBitcoinNetwork.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@utils.go`:
- Around line 151-160: The hardcoded knownNetworks slice duplicates logic from
toBitcoinNetwork and risks drifting; replace the inline knownNetworks usage in
the address validation loop with the canonical network set returned or
referenced by toBitcoinNetwork (e.g., iterate over the networks/mapping provided
by toBitcoinNetwork instead of creating &chaincfg.MainNetParams,
&chaincfg.TestNet3Params, &chaincfg.SigNetParams, &chaincfg.RegressionNetParams,
&arklib.MutinyNetSigNetParams), then call btcutil.DecodeAddress(address, net) as
before and return the same values; remove the duplicated slice so the code uses
the single source of truth in toBitcoinNetwork.
Tests cover valid onchain addresses (P2PKH, P2WPKH, P2TR across mainnet, testnet, and regtest), valid Ark offchain addresses, and invalid inputs (empty string, garbage, unknown prefix, truncated addresses). Addresses are generated programmatically to avoid hardcoded checksum values. https://claude.ai/code/session_0151wY6W7ib1h3HWof32Xz7q
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@utils_test.go`:
- Line 81: The long call at p2trMainnet, err :=
btcutil.NewAddressTaproot(schnorr.SerializePubKey(pubKey),
&chaincfg.MainNetParams) exceeds golines limits; split it across lines or
extract the serialized pubkey into a temporary variable to shorten the
expression. For example, assign serialized := schnorr.SerializePubKey(pubKey)
and then call btcutil.NewAddressTaproot(serialized, &chaincfg.MainNetParams) or
break the NewAddressTaproot call arguments onto multiple lines so the line
length constraint is satisfied (affects p2trMainnet, err,
btcutil.NewAddressTaproot, schnorr.SerializePubKey, pubKey,
chaincfg.MainNetParams).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| require.NoError(t, err) | ||
| p2wpkhMainnet, err := btcutil.NewAddressWitnessPubKeyHash(pubKeyHash, &chaincfg.MainNetParams) | ||
| require.NoError(t, err) | ||
| p2trMainnet, err := btcutil.NewAddressTaproot(schnorr.SerializePubKey(pubKey), &chaincfg.MainNetParams) |
There was a problem hiding this comment.
Fix golines formatting at Line 81 to unblock CI.
Line 81 exceeds formatting limits and is currently failing lint in CI.
Suggested formatting fix
- p2trMainnet, err := btcutil.NewAddressTaproot(schnorr.SerializePubKey(pubKey), &chaincfg.MainNetParams)
+ p2trMainnet, err := btcutil.NewAddressTaproot(
+ schnorr.SerializePubKey(pubKey),
+ &chaincfg.MainNetParams,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| p2trMainnet, err := btcutil.NewAddressTaproot(schnorr.SerializePubKey(pubKey), &chaincfg.MainNetParams) | |
| p2trMainnet, err := btcutil.NewAddressTaproot( | |
| schnorr.SerializePubKey(pubKey), | |
| &chaincfg.MainNetParams, | |
| ) |
🧰 Tools
🪛 GitHub Actions: ci_unit
[error] 81-81: golangci-lint reported formatting error: File is not properly formatted (golines).
🪛 GitHub Check: Sdk unit tests
[failure] 81-81:
File is not properly formatted (golines)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@utils_test.go` at line 81, The long call at p2trMainnet, err :=
btcutil.NewAddressTaproot(schnorr.SerializePubKey(pubKey),
&chaincfg.MainNetParams) exceeds golines limits; split it across lines or
extract the serialized pubkey into a temporary variable to shorten the
expression. For example, assign serialized := schnorr.SerializePubKey(pubKey)
and then call btcutil.NewAddressTaproot(serialized, &chaincfg.MainNetParams) or
break the NewAddressTaproot call arguments onto multiple lines so the line
length constraint is satisfied (affects p2trMainnet, err,
btcutil.NewAddressTaproot, schnorr.SerializePubKey, pubKey,
chaincfg.MainNetParams).
Contributor
|
@tiero this function should be added to arkd/pkg/ark-lib |
Member
Author
|
My idea was to |
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.
Adds an exported ValidateAddress(address string) function that infers
the network from the address prefix and returns whether it is a valid
Bitcoin onchain address or Ark offchain address, without requiring a
configured client.
https://claude.ai/code/session_0151wY6W7ib1h3HWof32Xz7q
Summary by CodeRabbit
New Features
Tests