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: split message to have dedicated to and from fields #6358

Merged
merged 4 commits into from
May 28, 2024

Conversation

SWvheerden
Copy link
Collaborator

Description

split message to have dedicated to and from fields

Motivation and Context

This is needed for #6353
When a chat client receives a message, it needs to know the complete address of the receiver, not just a part of it. This allows the user to get the complete address by saving what the sender said their address is

Copy link

github-actions bot commented May 27, 2024

Test Results (CI)

    3 files    120 suites   56m 20s ⏱️
1 275 tests 1 275 ✅ 0 💤 0 ❌
3 817 runs  3 817 ✅ 0 💤 0 ❌

Results for commit 64cbdc9.

♻️ This comment has been updated with latest results.

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels May 27, 2024
Copy link

github-actions bot commented May 27, 2024

Test Results (Integration tests)

 2 files  11 suites   40m 24s ⏱️
34 tests 32 ✅ 0 💤 2 ❌
37 runs  33 ✅ 0 💤 4 ❌

For more details on these failures, see this check.

Results for commit 64cbdc9.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@brianp brianp left a comment

Choose a reason for hiding this comment

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

It mostly looks good but I think some of the interface change goes in the wrong direction. It follows the pattern of providing readers for things, but then fails in practical usage.

brianp
brianp previously approved these changes May 28, 2024
Copy link
Contributor

@brianp brianp left a comment

Choose a reason for hiding this comment

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

LGTM

@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label May 28, 2024
make dual addresses

fmt

build
@SWvheerden SWvheerden force-pushed the sw_to_from_addresses branch from 03f5e90 to ca420a8 Compare May 28, 2024 07:14
@ghpbot-tari-project ghpbot-tari-project added the P-reviews_required Process - Requires a review from a lead maintainer to be merged label May 28, 2024
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

Looks good, just some comments about the naming convention.

base_layer/contacts/src/contacts_service/types/message.rs Outdated Show resolved Hide resolved
base_layer/contacts/proto/message.proto Outdated Show resolved Hide resolved
Comment on lines 43 to 44
pub to_address: Vec<u8>,
pub from_address: Vec<u8>,
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto receiver & sender

Comment on lines 57 to 58
pub to_address: Vec<u8>,
pub from_address: Vec<u8>,
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto receiver & sender

Comment on lines 167 to 170
let to_address =
TariAddress::from_bytes(&o.to_address).map_err(|_| ContactsServiceStorageError::ConversionError)?;
let from_address =
TariAddress::from_bytes(&o.from_address).map_err(|_| ContactsServiceStorageError::ConversionError)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto receiver & sender

Comment on lines 202 to 203
to_address: o.to_address.to_bytes().to_vec(),
from_address: o.from_address.to_bytes().to_vec(),
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto receiver & sender

SWvheerden and others added 3 commits May 28, 2024 13:51
Co-authored-by: Hansie Odendaal <39146854+hansieodendaal@users.noreply.github.com>
@SWvheerden SWvheerden merged commit c24cc15 into tari-project:development May 28, 2024
14 of 16 checks passed
@SWvheerden SWvheerden deleted the sw_to_from_addresses branch June 28, 2024 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants