Uh oh!
There was an error while loading. Please reload this page.
fix(dash-spv): preserve already defined change addr in the tx builder - #920
Open
ZocoLini wants to merge 1 commit into
Open
fix(dash-spv): preserve already defined change addr in the tx builder#920ZocoLini wants to merge 1 commit into
ZocoLini wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughChangesFunding change address
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`:
- Around line 127-129: Update TransactionBuilder::set_funding so it does not
call next_change_address or advance the change pool when initializing
change_addr; defer derivation until the build path has the final address, or
otherwise ensure set_change_address explicitly prevents and replaces any derived
address. Preserve the existing behavior for callers that do not provide a change
address, and add regression coverage for
set_funding(...).set_change_address(...).build_unsigned().
- Around line 127-129: Validate explicit change addresses in set_funding by
comparing the address network with funds_acc.network()/acc.network before
preserving change_addr, and return a new BuilderError variant on mismatch. Keep
matching explicit addresses unchanged, update
set_funding_keeps_an_explicit_change_address to use a matching-network
fixture/address, and add a regression test covering rejection of a cross-network
address.
🪄 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 Plus
Run ID: 16ccc41c-dfd2-40f9-a50f-b927f63181a9
📒 Files selected for processing (1)
key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
Comment on lines
+127
to
+129
| if self.change_addr.is_none() { | ||
| self.change_addr = funds_acc.next_change_address(Some(&acc.account_xpub), true).ok(); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bashset -euo pipefail
# Inspect builder chains and address-pool mutations.
rg -n -C 8 'set_funding|set_change_address|next_change_address' \
key-wallet/src --glob '*.rs'Repository: dashpay/rust-dashcore
Length of output: 50378
🏁 Script executed:
#!/usr/bin/env bashset -euo pipefail
# Map the transaction_builder source and inspect relevant builder implementation plus tests.
wc -l key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
sed -n '1,170p' key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
sed -n '1210,1305p' key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
# Find all TransactionBuilder chain calls in key-wallet source without tests/context noise.printf'\n--- builder chains in key-wallet src ---\n'
python3 - <<'PY'from pathlib import Pathimport reroot = Path('key-wallet/src')pairs = set()for path in root.rglob('*.rs'): text = path.read_text(errors='replace') for m in re.finditer(r'TransactionBuilder::new\(\)', text): start = m.start() line_start = text.rfind('\n', 0, start) + 1 line_end = text.find('\n', start + 2000) text = text if line_end < 0 else text[:line_end] + text[line_end+1:] chain = text[start:start+2000] h1 = 'set_funding' in chain h2 = 'set_change_address' in chain if h1 or h2: # capture minimal surrounding block up to next semicolon/new call-ish boundary out = [] for ch in chain: out.append(ch) if ch == '.': break out.append('...') pairs.add((int(text[:start].count('\n')+1), path.as_posix(), out[:200])) # stop at first full builder chain block breakprint('\n'.join(f"{ln:5} {path} {chain}" for ln, path, chain in sorted(pairs)))PYRepository: dashpay/rust-dashcore
Length of output: 11046
🏁 Script executed:
#!/usr/bin/env bashset -euo pipefail
printf'%s\n''--- all TransactionBuilder builder chains in key-wallet/src ---'
python3 - <<'PY'import refrom pathlib import Pathfor path in Path('key-wallet/src').rglob('*.rs'): text = path.read_text(errors='replace') for m in re.finditer(r'TransactionBuilder::new\(\)', text): line = text[:m.start()].count('\n') + 1 end = min(len(text), m.start() + 2000) chunk = text[m.start():end] if 'set_funding' in chunk or 'set_change_address' in chunk: print(f'{path}:{line}:') for i, block in enumerate(chunk.split('\n'), 1): print(f' {block}') breakPYprintf'%s\n''--- ManagedCoreFundsAccount next_change_address definitions/usages ---'
rg -n -C 5 'fn next_change_address|next_receive_address|advance.*change|change_addr|ChangeAddresses' key-wallet/src --glob '*.rs'| head -n 220
printf'%s\n''--- focused builder chain occurrences ---'
rg -n -C 3 'set_funding|set_change_address' key-wallet/src/wallet/managed_wallet_info/transaction_builder.rsRepository: dashpay/rust-dashcore
Length of output: 35390
Preserve next_change_address calls from advancing the change pool.
set_funding calls funds_acc.next_change_address(..., true), but later .set_change_address(...) can replace change_addr. Existing chains use this order, including production calls in TransactionBuilder and build_and_sign_transaction_with_signer, so one change address can be consumed and discarded. Defer change derivation until the build path has the final address, or make the setter order explicit and add a regression covering .set_funding(...).set_change_address(...).build_unsigned().
🤖 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 `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs` around
lines 127 - 129, Update TransactionBuilder::set_funding so it does not call
next_change_address or advance the change pool when initializing change_addr;
defer derivation until the build path has the final address, or otherwise ensure
set_change_address explicitly prevents and replaces any derived address.
Preserve the existing behavior for callers that do not provide a change address,
and add regression coverage for
set_funding(...).set_change_address(...).build_unsigned().
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bashset -euo pipefail
# Locate the account network source and address-validation helpers.
rg -n -C 8 -P 'struct\s+ManagedCoreFundsAccount|network|is_valid_for_network|Network::' \
key-wallet/src/managed_account key-wallet/src/wallet --glob '*.rs'# Confirm the networks used by the regression fixture.
rg -n -C 6 -P 'dummy_bip44|Address::dummy\(Network::(Testnet|Regtest)' \
key-wallet/src --glob '*.rs'Repository: dashpay/rust-dashcore
Length of output: 50378
🏁 Script executed:
#!/usr/bin/env bashset -euo pipefail
# Inspect the builder state initialization, network source methods, change address handling,# and the specific regression test around lines 127 and 1218-1232.
sed -n '1,180p' key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
sed -n '118,200p' key-wallet/src/managed_account/managed_funds_account.rs
sed -n '55,95p' key-wallet/src/managed_account/managed_account_ref.rs
sed -n '1215,1235p' key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
# Find address validation helpers and error text related to network/address validation.
rg -n -C 4 'fn .*network|is_.+_network|NetworkMismatch|invalid.*network|invalid_address|validate.*address' key-wallet/src --glob '*.rs'Repository: dashpay/rust-dashcore
Length of output: 7193
🏁 Script executed:
#!/usr/bin/env bashset -euo pipefail
# Locate the managed funds account source and test for address network validation helpers.
fd -a '.*managed.*funds.*\.rs|.*account.*ref.*\.rs|.*funding.*account.*\.rs' key-wallet/src ||true
rg -n -C 8 'ManagedCoreFundsAccount|dummy_bip44|fn network\(|is_.*network|validate_|Address::dummy\(Network::Regtest|set_change_address' key-wallet/src --glob '*.rs'| sed -n '1,220p'# Inspect the exact regression test.
sed -n '1215,1235p' key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
# Locate where transaction builder obtains the funds account/network during build.
rg -n -C 6 'fn build_signed|fn build_unsigned|ManagedCoreFundsAccount|funding_account|set_funding|next_change_address|change_addr' key-wallet/src/wallet/managed_wallet_info/transaction_builder.rsRepository: dashpay/rust-dashcore
Length of output: 41463
🏁 Script executed:
#!/usr/bin/env bashset -euo pipefail
# Inspect the managed funds account implementation and network helpers.
sed -n '1,220p' key-wallet/src/managed_account/managed_core_funds_account.rs
sed -n '1,180p' key-wallet/src/managed_account/managed_account_ref.rs
# Search narrowly for network validation helpers in managed account types.
rg -n -C 4 'fn network|network\s*:\s*Network|is_.*network|is_valid_for_network|NetworkMismatch|invalid.*network|Address::dummy\(Network::Regtest' key-wallet/src/managed_account --glob '*.rs'# Search narrowly for error kinds that the transaction builder should use.
rg -n -C 3 'enum BuilderError|BuilderError::|Change|change_addr|Network' key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs keRepository: dashpay/rust-dashcore
Length of output: 50377
🏁 Script executed:
#!/usr/bin/env bashset -euo pipefail
# Locate the transaction builder error enum and builder construction/entrypoints to see where funding/account network can be compared.
rg -n -C 5 'enum BuilderError|NetworkMismatch|Invalid.*Network|change_addr|set_funding\(.*Account|set_funding\(&mut funds_acc' key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
# Locate ManagedCoreFundsAccount constructor methods to confirm the dummy fixture network.
rg -n -C 6 'impl ManagedCoreFundsAccount|pub fn dummy_bip44|managed_account_type\(\)\.into\(\)|Network::Regtest|Network::Testnet' key-wallet/src/managed_account/managed_core_funds_account.rs
# Locate Account network source.
rg -n -C 4 'struct Account|impl AccountTrait for Account|fn network\(&self\)|network:' key-wallet/src/account --glob '*.rs'Repository: dashpay/rust-dashcore
Length of output: 34445
Reject a cross-network explicit change address in set_change_address.
set_funding only preserves an already-set change_addr, while assemble_unsigned later emits change_addr.script_pubkey() without validating network membership. In the explicit-address path, builder.network() is unavailable, but set_funding can compare the provided change address against funds_acc.network()/acc.network and add a BuilderError variant for mismatch.
Also update set_funding_keeps_an_explicit_change_address to use the fixture network or construct a matching address explicitly, and add a regression test case that rejects a mismatched explicit change address.
🤖 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 `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs` around
lines 127 - 129, Validate explicit change addresses in set_funding by comparing
the address network with funds_acc.network()/acc.network before preserving
change_addr, and return a new BuilderError variant on mismatch. Keep matching
explicit addresses unchanged, update
set_funding_keeps_an_explicit_change_address to use a matching-network
fixture/address, and add a regression test covering rejection of a cross-network
address.
Source: Coding guidelines
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## dev #920 +/- ##
==========================================
- Coverage 74.76% 74.74% -0.03%
==========================================
Files 328 328 Lines 76593 76613 +20 ==========================================
- Hits 57267 57266 -1 - Misses 19326 19347 +21
|
Contributor
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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