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: refactor configuration for chat so ffi can create and accept a config file #5426

Merged
merged 4 commits into from
May 30, 2023

Conversation

brianp
Copy link
Contributor

@brianp brianp commented May 30, 2023

Description

This refactoring come on the heels of making testing easier via the chat configuration. As a result we also solve the problem about peer seeds in FFI. Previously the ffi required passing in a collection of peer seeds. This wasn't in any way ideal. Now we take a standard tari configuration setup, which will utilize the DNS seeds in the default configuration.

Motivation and Context

ChatFFI had a bad way to connect to peers. It needed fixing. Also cleanup of the integration tests was desired and making configuration simpler is helpful.

How Has This Been Tested?

Locally and CI

No new test was added for the configuration creation yet. As that config defaults to real config, not local config.

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

Breaking Changes

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

brianp added 4 commits May 30, 2023 13:23
The config contains all other options we need. Peer seeds, Network, TCP
info etc. This will aid in solving the problem with the chat ffi where
we need to pass in a peer list. That's not easily solved from mobile.
Supporting the standard config loader means we can use the config file
generated from the wallet or any standard Tari app.
Instead of seed peers and a p2p config, accept a chat specific config
that the ffi can also produce.
@github-actions
Copy link

Test Results (CI)

1 148 tests  ±0   1 148 ✔️ ±0   10m 20s ⏱️ +14s
     37 suites ±0          0 💤 ±0 
       1 files   ±0          0 ±0 

Results for commit 39e6d3b. ± Comparison against base commit c29ab15.

@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 30, 2023
@github-actions
Copy link

Test Results (Integration tests)

  2 files  ±0  12 suites  ±0   44m 7s ⏱️ + 9m 40s
29 tests ±0  27 ✔️ ±0  0 💤 ±0  2 ±0 
33 runs  +1  28 ✔️  - 1  0 💤 ±0  5 +2 

For more details on these failures, see this check.

Results for commit 39e6d3b. ± Comparison against base commit c29ab15.

@brianp brianp requested a review from SWvheerden May 30, 2023 13:33
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.

Ack

@@ -14,6 +14,7 @@ members = [
"base_layer/tari_mining_helper_ffi",
"clients/rust/base_node_grpc_client",
"clients/rust/wallet_grpc_client",
# "clients/rust/web_chat",
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove?

@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label May 30, 2023
@SWvheerden SWvheerden merged commit 9d0d8b5 into tari-project:development May 30, 2023
@brianp brianp deleted the refactor-chat branch May 31, 2023 07:02
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.

3 participants