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: chat scaffold #5244

Merged
merged 20 commits into from
Apr 18, 2023
Merged

Conversation

brianp
Copy link
Contributor

@brianp brianp commented Mar 13, 2023

Description

  • Refactor the contacts crate for better organization.
  • Expand contacts with types, and capabilities for managing messaging over the existing comms network
  • Add cucumber tests to provide support for new functionality

Motivation and Context

We want to expand Tari's communication network to handle p2p chat services. This PR sets us off in the right direction for the core Tari processes to be able to handle message passing and storage. Currently chat clients will be able to send, and receive messages when online or offline. Utilizing store and forward for sending communications offline.

How Has This Been Tested?

Via cucumber only. The chat client within cucumber is currently the only interface for trials.

Caveats and known issues

  • Encryption at rest: We need to change the message storage to support encryption at rest

@brianp brianp changed the title feature: wip chat scaffold feat: wip chat scaffold Mar 13, 2023
@brianp brianp force-pushed the feature-chat-scaffold branch 2 times, most recently from 2ee0044 to 300fa2d Compare March 14, 2023 13:10
@brianp brianp force-pushed the feature-chat-scaffold branch 3 times, most recently from 1c8cd19 to deffd19 Compare March 29, 2023 15:27
@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 Mar 29, 2023
@brianp brianp force-pushed the feature-chat-scaffold branch 2 times, most recently from ee8560f to 848d3d7 Compare March 29, 2023 15:44
@brianp brianp closed this Mar 31, 2023
@brianp brianp deleted the feature-chat-scaffold branch March 31, 2023 08:18
@brianp brianp restored the feature-chat-scaffold branch March 31, 2023 08:19
@brianp brianp reopened this Mar 31, 2023
@brianp brianp force-pushed the feature-chat-scaffold branch 2 times, most recently from a8259ea to e81351a Compare March 31, 2023 21:36
@brianp brianp marked this pull request as ready for review March 31, 2023 21:36
@brianp brianp changed the title feat: wip chat scaffold feat: chat scaffold Apr 2, 2023
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.

Looking good, I just have some comments about the cucumber scenarios.

integration_tests/tests/features/Chat.feature Outdated Show resolved Hide resolved
Comment on lines +14 to +16
Given I have a seed node SEED_A
When I have a chat client CHAT_A connected to seed node SEED_A
When I have a chat client CHAT_B connected to seed node SEED_A
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it still necessary to have the 3rd party base node here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without the seed node the two chat clients don't seem to find each other after being added as contacts. We would probably need to introduce a peer dialing step for new contacts if the client doesn't otherwise have connectivity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can register an issue to monitor if this will be a problem for the mobile wallets, as they will most probably not connect to the same base node at startup.

integration_tests/tests/features/Chat.feature Show resolved Hide resolved
integration_tests/tests/features/Chat.feature Show resolved Hide resolved
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.

This is looking good

applications/tari_base_node/log4rs_sample.yml Outdated Show resolved Hide resolved
base_layer/contacts/src/contacts_service/handle.rs Outdated Show resolved Hide resolved
base_layer/contacts/src/contacts_service/service.rs Outdated Show resolved Hide resolved
Comment on lines +41 to +42
pub address: Vec<u8>,
node_id: Vec<u8>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Whats the reason we have both here?
address = [node_id][CRC + network]
Asking the node id from address is a simple slice read

Comment on lines +30 to +31
pub address: TariAddress,
pub node_id: NodeId,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I asked above, but same here, why store both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, not sure. This code was moved in the refactor, not written new for this PR.

comms/dht/src/outbound/requester.rs Outdated Show resolved Hide resolved
integration_tests/log4rs/wallet.yml Show resolved Hide resolved
This refactor reorganizes the contacts crate and builds out the
possibility of sending messages to contacts via the already running
comms services. Messages will be monitored for and stored locally for
the client to retrieve and present in whatever form.
These references are out of date after the refactor and just needed a
little guidance.
This provides a barebones chat client used for testing within cucumber.
The client is relatively isolated from the test suite itself and could
be expanded on in the future.
I was using the default encryption method, which happens to be
plaintext. This caused all messages to go out unencrypted and allowed
all clients to receive and read all messages. With the appropriate
encryption methods client can't read messages not for them, and ignore
them, or SAF.
Update the log4s to include the refactoring of contacts and surrounding
services.
The ids shouldn't increment per client this would be a useless function.
Having a single message between two clients share a common uuid
increases ease of integration for future functionality such as
validation messages have arrived and read receipts etc.
Instead of changing the default impl of an existing function, move the
old function to the unencrypted name, make the new function with an
encrypted name, and let any call to the old api fail.
@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Apr 18, 2023
@SWvheerden
Copy link
Collaborator

Ack

@SWvheerden SWvheerden merged commit 5b09f8e into tari-project:development Apr 18, 2023
SWvheerden added a commit that referenced this pull request May 8, 2023
##
[0.50.0-pre.1](v0.50.0-pre.0...v0.50.0-pre.1)
(2023-05-08)


### Features

* add miner timeout config option
([5331](#5331))
([aea14f6](aea14f6))
* chat ffi ([5349](#5349))
([f7cece2](f7cece2))
* chat scaffold
([5244](#5244))
([5b09f8e](5b09f8e))
* improve message encryption
([5288](#5288))
([7a80716](7a80716))
* **p2p:** allow listener bind to differ from the tor forward address
([5357](#5357))
([857fb55](857fb55))


### Bug Fixes

* add SECURITY.md Vulnerability Disclosure Policy
([5351](#5351))
([72daaf5](72daaf5))
* added missing log4rs features
([5356](#5356))
([b9031bb](b9031bb))
* allow public addresses from command line
([5303](#5303))
([349ac89](349ac89))
* clippy issues with config
([5334](#5334))
([026f0d5](026f0d5))
* default network selection
([5333](#5333))
([cf4b2c8](cf4b2c8))
* make the first output optional in the wallet
([5352](#5352))
([bf16140](bf16140))
* remove wallet panic
([5338](#5338))
([536d16d](536d16d))
* wallet .h file for lib wallets
([5330](#5330))
([22a3a17](22a3a17))
@brianp brianp deleted the feature-chat-scaffold branch October 2, 2023 14:31
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants