test: extract wallet setup into TestWalletContext#539
Open
xdustinface wants to merge 1 commit intov0.42-devfrom
Open
test: extract wallet setup into TestWalletContext#539xdustinface wants to merge 1 commit intov0.42-devfrom
TestWalletContext#539xdustinface wants to merge 1 commit intov0.42-devfrom
Conversation
Contributor
📝 WalkthroughWalkthroughA new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
xdustinface
changed the title
TestWalletContextTestWalletContext
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #539 +/- ##
=============================================
- Coverage 66.89% 66.85% -0.05%
=============================================
Files 313 313
Lines 64753 64701 -52
=============================================
- Hits 43317 43254 -63
- Misses 21436 21447 +11
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
xdustinface
force-pushed
the
test/create-transaction
branch
from
0c394c7 to
0077e89
Compare
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
Add `TestWalletContext` struct to `test_utils::wallet` providing a pre-built testnet wallet with BIP44 account, receive address, and xpub. Replace duplicated wallet setup code across `wallet_checker`, `routing`, `coinbase`, and `asset_unlock` test modules.
xdustinface
force-pushed
the
test/wallet-context
branch
from
f60fae9 to
c9e13c1
Compare
github-actions
bot
removed
the
merge-conflict
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
key-wallet/src/test_utils/wallet.rs (1)
24-44: Make this helper's BIP44-only contract explicit.
new_random()hardcodesWalletAccountCreationOptions::Defaultand then immediately binds tostandard_bip44_accounts.get(&0), so this is not a general wallet fixture. That ambiguity already lettest_asset_unlock_routing_to_bip32_accountswitch to a BIP44 address. A more specific name, or separate builders for BIP44/BIP32/CoinJoin contexts, would make that kind of coverage loss much harder to introduce.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/test_utils/wallet.rs` around lines 24 - 44, The helper TestWalletContext::new_random() currently creates a wallet with WalletAccountCreationOptions::Default and immediately uses standard_bip44_accounts.get(&0), implying it is BIP44-only; rename or split it to make that explicit (e.g., new_random_bip44) or add an explicit parameter for WalletAccountCreationOptions so callers request BIP44 vs BIP32/CoinJoin, and update the function body to assert/use the chosen option (references: TestWalletContext::new_random, WalletAccountCreationOptions::Default, standard_bip44_accounts.get(&0), ManagedWalletInfo::from_wallet_with_name, first_bip44_managed_account_mut); ensure tests adjust to call the new BIP44-specific helper or pass the BIP44 option.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@key-wallet/src/transaction_checking/transaction_router/tests/asset_unlock.rs`:
- Around line 141-146: The test no longer covers BIP32 because it calls
TestWalletContext::new_random() which provisions a BIP44 receive address and the
assertion expects AccountTypeToCheck::StandardBIP44; restore explicit BIP32
setup by replacing the new_random() call with a helper that provisions a BIP32
receive address (or extend TestWalletContext to expose a BIP32 address), ensure
the test variables (managed_wallet_info, wallet, receive_address) refer to that
BIP32 account, and update the later assertion in
test_asset_unlock_routing_to_bip32_account to expect the BIP32
AccountTypeToCheck variant instead of StandardBIP44.
---
Nitpick comments:
In `@key-wallet/src/test_utils/wallet.rs`:
- Around line 24-44: The helper TestWalletContext::new_random() currently
creates a wallet with WalletAccountCreationOptions::Default and immediately uses
standard_bip44_accounts.get(&0), implying it is BIP44-only; rename or split it
to make that explicit (e.g., new_random_bip44) or add an explicit parameter for
WalletAccountCreationOptions so callers request BIP44 vs BIP32/CoinJoin, and
update the function body to assert/use the chosen option (references:
TestWalletContext::new_random, WalletAccountCreationOptions::Default,
standard_bip44_accounts.get(&0), ManagedWalletInfo::from_wallet_with_name,
first_bip44_managed_account_mut); ensure tests adjust to call the new
BIP44-specific helper or pass the BIP44 option.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 995f992f-ca10-43c9-8c78-5b86b489ea82
📒 Files selected for processing (6)
key-wallet/src/test_utils/mod.rskey-wallet/src/test_utils/wallet.rskey-wallet/src/transaction_checking/transaction_router/tests/asset_unlock.rskey-wallet/src/transaction_checking/transaction_router/tests/coinbase.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet/src/transaction_checking/wallet_checker.rs
key-wallet/src/transaction_checking/transaction_router/tests/asset_unlock.rs
Show resolved
Hide resolved
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
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.
Add
TestWalletContextstruct totest_utils::walletproviding a pre-built testnet wallet with BIP44 account, receive address, and xpub. Replace duplicated wallet setup code acrosswallet_checker,routing,coinbase, andasset_unlocktest modules.Summary by CodeRabbit