feat: overrride coinbase payment_id if included in wallet payment address#7038
Conversation
|
""" WalkthroughThis update refactors payment ID handling across several modules by renaming methods to clarify their association with user data. It introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant WalletAddress
participant CoinbaseBuilder
Caller->>WalletAddress: get_payment_id_user_data_bytes()
alt If user data is present
WalletAddress-->>Caller: payment_id_bytes
Caller->>CoinbaseBuilder: generate_coinbase_with_wallet_output(..., payment_id=empty)
CoinbaseBuilder->>WalletAddress: get_payment_id_user_data_bytes()
CoinbaseBuilder-->>Caller: Output with payment_id = user data
else No user data
WalletAddress-->>Caller: empty
Caller->>CoinbaseBuilder: generate_coinbase_with_wallet_output(..., payment_id)
CoinbaseBuilder-->>Caller: Output with provided payment_id
end
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Test Results (Integration tests) 2 files 11 suites 1h 38m 39s ⏱️ For more details on these failures, see this check. Results for commit f6250af. ♻️ This comment has been updated with latest results. |
Test Results (CI)1 083 tests 1 078 ✅ 15m 58s ⏱️ For more details on these failures, see this check. Results for commit f6250af. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
base_layer/core/src/transactions/coinbase_builder.rs (1)
452-460: New feature to override payment ID from wallet address.This is the core functionality change that checks if the wallet address contains a payment ID user data and uses it to override the provided payment ID for coinbase transactions. This will be useful for pool operators as mentioned in the PR description.
There's a minor typo in the comment on line 452: "pyment" should be "payment".
- // Override payment id if the wallet address contains a pyment id + // Override payment id if the wallet address contains a payment id
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(1 hunks)base_layer/common_types/src/tari_address/dual_address.rs(2 hunks)base_layer/common_types/src/tari_address/mod.rs(1 hunks)base_layer/core/src/transactions/coinbase_builder.rs(5 hunks)base_layer/core/src/transactions/transaction_components/encrypted_data.rs(7 hunks)base_layer/wallet/src/transaction_service/service.rs(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
base_layer/wallet/src/transaction_service/service.rs (1)
base_layer/core/src/transactions/transaction_components/encrypted_data.rs (1)
open(566-571)
base_layer/common_types/src/tari_address/dual_address.rs (1)
base_layer/common_types/src/tari_address/mod.rs (1)
get_payment_id_user_data_bytes(242-247)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
- GitHub Check: ci
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (testnet, esmeralda)
🔇 Additional comments (18)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (2)
275-276: Method name update for consistency.The method name change from
create_payment_id_addresstowith_payment_id_user_datareflects a more accurate description of the functionality and maintains consistent naming across the codebase.
283-284: Method name update for consistency.Similar to the change above, this method name change improves clarity by explicitly indicating that the payment ID is being stored as user data in the address.
base_layer/wallet/src/transaction_service/service.rs (2)
1209-1209: Method rename to clarify user data context.The method has been renamed from
get_payment_id_bytes()toget_payment_id_user_data_bytes()to better describe that these bytes come from user data embedded in the address.
1764-1764: Method rename to clarify user data context.The method has been renamed from
get_payment_id_bytes()toget_payment_id_user_data_bytes()to better describe that these bytes come from user data embedded in the address. This change is consistent with the update insend_transaction().base_layer/common_types/src/tari_address/dual_address.rs (2)
92-99: Method name change clarifies purpose.The method rename from
add_payment_idtoadd_payment_id_user_databetter reflects that this method deals with user data related to payment IDs rather than payment IDs themselves. This naming is more precise and consistent with the overall payment ID refactoring.
129-131: Method name change clarifies purpose.Renaming
get_payment_id_bytestoget_payment_id_user_data_bytesimproves clarity by explicitly stating that the method returns user data bytes associated with a payment ID, not the payment ID itself. This change aligns with the corresponding method renamed in the TariAddress enum.base_layer/core/src/transactions/transaction_components/encrypted_data.rs (7)
96-96: New Coinbase transaction type added.The addition of a
Coinbasevariant with bit pattern0b1011supports the PR objective of enhancing coinbase transaction generation by providing a dedicated transaction type.
116-116: Coinbase variant properly implemented in from_u16 method.The new Coinbase transaction type is correctly handled in the conversion from u16 values, maintaining consistency with the type system.
133-133: Coinbase variant properly implemented in as_u8 method.The serialization of the Coinbase transaction type is correctly implemented to return its binary representation
0b1011.
155-155: Display implementation for Coinbase transaction type.The Display trait implementation is properly updated to handle the new Coinbase variant, ensuring consistent string representation.
1105-1106: Test cases updated for Coinbase transaction type.The test cases have been properly updated to include the new Coinbase transaction type, ensuring proper test coverage for the new functionality.
1227-1228: Coinbase variant included in serialization tests.The Coinbase transaction type is correctly included in the serialization/deserialization tests, ensuring that it works properly with the existing serialization mechanisms.
1372-1388: Fee handling tests updated for Coinbase transaction type.Tests for metadata handling with overflow conditions have been updated to use the new Coinbase transaction type, ensuring consistency throughout the test suite.
base_layer/common_types/src/tari_address/mod.rs (2)
242-247: Method name change for consistency.Renaming
get_payment_id_bytestoget_payment_id_user_data_bytesin the TariAddress enum maintains consistency with the underlying DualAddress implementation and clarifies the method purpose. The implementation correctly delegates to the renamed method in DualAddress.
249-258: Method name change improves clarity.Renaming
create_payment_id_addresstowith_payment_id_user_databetter describes the method's functionality as adding user data to an existing address rather than creating a new address. The implementation correctly calls the renamed method in DualAddress.base_layer/core/src/transactions/coinbase_builder.rs (3)
44-44: Update to import dependencies for payment ID handling.This change adds the import of
TxTypeenum which is needed for the new payment ID override functionality.
794-814: Updated imports for test module.Added necessary imports to support the new test function.
1234-1281: Good test coverage for the new payment ID override functionality.This test thoroughly verifies that when a wallet address with an embedded payment ID is used, the coinbase transaction adopts that payment ID instead of the empty one provided as an argument. The test creates an address with custom payment ID user data, then confirms the generated coinbase output contains that same data.
Added coinbase payment_id to thre coinbase from the wallet payment address if it exists.
hansieodendaal
force-pushed
the
ho_coinbase_payment_id
branch
from
d662466 to
8778fee
Compare
2 participants
Description
Added coinbase payment_id to the coinbase from the wallet payment address if it exists.
Motivation and Context
This functionality can be used by pool operators.
How Has This Been Tested?
Added new unit test -
test_generate_coinbase_with_payment_id_from_addressWhat process can a PR reviewer use to test or verify this change?
Code review
Breaking Changes
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor