refactor(key-wallet): drop function to create unisgned transactions in WalletManager and in ManagedWalletInfo#483
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
💤 Files with no reviewable changes (2)
📝 WalkthroughWalkthroughRemoved unsigned-payment-transaction creation: deleted wallet-manager adapter and module declaration, removed WalletInfoInterface method, and removed ManagedWalletInfo's internal transaction-building implementation and related tests/imports. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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)
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 |
ZocoLini
force-pushed
the
refactor/drop-unsigned-tx-building
branch
from
f6839ff to
77f3a5a
Compare
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
You should remove the wallet_manager::transaction_building.rs file and then TransactionError becomes unused and managed_wallet_info::transaction_building.rs is only left with AccountTypePreference (and the tests). Maybe good to move that struct then to a more appropriate place then here or maybe as follow up.
Also the tests in managed_wallet_info::transaction_building.rs should be moved/merged/deduped with the transaction builder tests then here or as follow up.
|
|
||
| mod matching; | ||
| mod process_block; | ||
| mod transaction_building; |
There was a problem hiding this comment.
You removing the module here but keeping the file which now is dead code and also is trying to access dead code.
There was a problem hiding this comment.
oh, yeah, didnt notice, ty
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
@CodeRabbit why do care about my rate limit in @ZocoLini's PR? |
|
Tip For best results, initiate chat on the files or code changes.
This design ensures fair resource allocation across all users—regardless of whose PR you're reviewing, manual review requests consume the quota of the person making the request. This prevents any single user from overwhelming the system by triggering excessive reviews, even on other people's PRs. Once the rate limit timer resets (in about 9 minutes from your last request), you'll be able to trigger manual reviews again. 🧠 Learnings used |
|
@CodeRabbit review now, limit should be cleared. |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-wallet/src/wallet/managed_wallet_info/transaction_building.rs (1)
5-31:⚠️ Potential issue | 🟡 MinorRemove the unused
TransactionErrorenum.
AccountTypePreferenceis actively used throughoutkey-wallet-manager, including in address derivation methods and integration tests, so it should be retained. However,TransactionErrorshows no external usage and appears to be dead code following the removal ofcreate_unsigned_payment_transaction_internal. Remove it to keep the codebase clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/wallet/managed_wallet_info/transaction_building.rs` around lines 5 - 31, The TransactionError enum is dead code and should be removed to clean up the module: delete the entire TransactionError definition (the enum and its variants) from transaction_building.rs, ensuring you do not touch or remove AccountTypePreference (which is still used elsewhere) and verify there are no remaining references to TransactionError or to the removed create_unsigned_payment_transaction_internal function; if any references exist, update callers to use the new error handling or remove those callers accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@key-wallet/src/wallet/managed_wallet_info/transaction_building.rs`:
- Around line 5-31: The TransactionError enum is dead code and should be removed
to clean up the module: delete the entire TransactionError definition (the enum
and its variants) from transaction_building.rs, ensuring you do not touch or
remove AccountTypePreference (which is still used elsewhere) and verify there
are no remaining references to TransactionError or to the removed
create_unsigned_payment_transaction_internal function; if any references exist,
update callers to use the new error handling or remove those callers
accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
key-wallet-manager/src/wallet_manager/mod.rskey-wallet/src/wallet/managed_wallet_info/transaction_building.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
💤 Files with no reviewable changes (1)
- key-wallet-manager/src/wallet_manager/mod.rs
|
I will continue working on the transaction related stuff reorganizing and clearing, will refactor the stuff you did mention in following PRs, I want to preoritize TransactionBuilder and the logic behind building signed and unsigned transactions |
ZocoLini
force-pushed
the
refactor/drop-unsigned-tx-building
branch
from
77f3a5a to
f956b47
Compare
… ManagedWalletInfo
ZocoLini
force-pushed
the
refactor/drop-unsigned-tx-building
branch
from
f956b47 to
614b36e
Compare
|
NVM, i dropped the error, but will leave AccountTypePreference and the tests for a following PR |
2 participants
I am removing this with the idea of improving TransactionBuilder struct logic and let it handle how to build signed/unsigned transaction, it makes more sense to me to use the TransactionBuilder since we already have one
Summary by CodeRabbit