Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: add change metadata to encrypted data #6713

Merged

Conversation

hansieodendaal
Copy link
Contributor

@hansieodendaal hansieodendaal commented Dec 5, 2024

Description

  • Added receiver address, amount, and payment ID to encrypted data so a view-only wallet can identify the receiver when importing a change output.
  • Fixed payment ID not propagated for interactive transactions.
  • Added payment ID to burn transactions.
  • Edit Removed message from transactions - payment ID is now used consistently throughout

Note: Burn transaction status for change outputs needs to be fixed, see #6726

Motivation and Context

Change data needs to be available for wallet recovery. This is especially useful for view-only wallets.

How Has This Been Tested?

System-level testing

one-sided

  • Sender wallet, one-sided
    image

  • Receiver wallet, one-sided
    image

  • View-only wallet, one-sided
    image

interactive

  • Sender wallet, interactive
    image

  • Receiver wallet, interactive
    image

  • View-only wallet, interactive
    image

burn

  • Sender wallet, burn
    image

  • View-only wallet, burn
    image

What process can a PR reviewer use to test or verify this change?

Code review
System-level testing

Breaking Changes

  • None
  • Requires data directory on base node to be deleted
  • Requires hard fork
  • Other - Please specify

BREAKING CHANGE:

  • Console wallet command line arguments:
    • --message is not supported anymore, only --payment-id
  • Wallet FFI:
    • fn completed_transaction_get_message( removed
    • fn pending_outbound_transaction_get_message( removed
    • fn pending_outbound_transaction_get_payment_id( added
    • fn pending_inbound_transaction_get_message( removed
    • fn pending_inbound_transaction_get_payment_id( added
    • fn wallet_send_transaction( has interface change
  • Wallet gRPC methods:
    • message CreateBurnTransactionRequest { has interface change
    • message PaymentRecipient { has interface change
    • message TransactionInfo { has interface change
    • message CoinSplitRequest { has interface change
    • message ImportUtxosRequest { has interface change
    • message TransactionEvent { has interface change
    • message RegisterValidatorNodeRequest { has interface change

@hansieodendaal hansieodendaal requested a review from a team as a code owner December 5, 2024 14:00
Copy link

github-actions bot commented Dec 5, 2024

Test Results (CI)

    3 files    129 suites   36m 11s ⏱️
1 349 tests 1 349 ✅ 0 💤 0 ❌
4 045 runs  4 045 ✅ 0 💤 0 ❌

Results for commit d54da49.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Dec 5, 2024

Test Results (Integration tests)

36 tests  +36   36 ✅ +36   15m 20s ⏱️ + 15m 20s
11 suites +11    0 💤 ± 0 
 2 files   + 2    0 ❌ ± 0 

Results for commit d54da49. ± Comparison against base commit 37c30be.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@SWvheerden SWvheerden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this PR moves in the right direction there are a few issues here:
1: Payment_id != message. There can be a message inside the payment_id, but its very much user data(message) + system level data. We need to keep separation between the two. This PR completely merges it and makes it all user data. Looking at the data I suspect there might have been some regression in terms of display here.

2: On the transmission of the data, the sender should not and can not control what the receiver pputs in his/her output. So no use in sending anything like that over. That data is sent over, but never actually used in the output. Interactive payments should keep using only the message. Dont add payment id for the receiver there.

3: On the display, we should not just display the raw data on the transactions.
For example on the view only wallet. It should not display the incoming data. It should display output going transaction for that amount. Not incoming. It should be more clever how it creates the transaction. It should read the details from the payment id field to create the outgoing tx. It should look almost similar to the spending wallet

4: This is a regression by the looks of it, but we should not display default addresses. We should self, or something like that there.

@hansieodendaal hansieodendaal changed the title feat: add change metadata to encrypted data feat!: add change metadata to encrypted data Dec 10, 2024
@hansieodendaal hansieodendaal force-pushed the ho_encrypted_addresses branch 3 times, most recently from d92486a to 524b36e Compare December 12, 2024 20:15
Added receiver address. amount and payment id to encrypted data so a view only
wallet will be able to identify the receiver when importing a change output.
Copy link
Collaborator

@SWvheerden SWvheerden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think is can go in like this for now

PaymentId::Empty => self.to_string(),
PaymentId::U64(v) => format!("{}", v),
PaymentId::U256(v) => format!("{}", v),
PaymentId::Address(v) => v.to_base58(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

II think this one should probably also display nothing, but its fine for now

@SWvheerden SWvheerden merged commit 686e7fd into tari-project:development Dec 13, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants