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

tx5 ep3 integration of endpoint/connection state management refactor #3287

Merged
merged 11 commits into from
Feb 6, 2024

Conversation

neonphog
Copy link
Contributor

@neonphog neonphog commented Feb 1, 2024

Summary

TODO:

  • CHANGELOG(s) updated with appropriate info
  • Just before pressing the merge button, ensure new entries to CHANGELOG(s) are still under the UNRELEASED heading

@neonphog neonphog changed the title wip tx5 ep3 integration tx5 ep3 integration of endpoint/connection state management refactor Feb 6, 2024
@neonphog neonphog marked this pull request as ready for review February 6, 2024 17:21
Copy link
Member

@ThetaSinner ThetaSinner left a comment

Choose a reason for hiding this comment

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

🥳

@@ -7,6 +7,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).

## \[Unreleased\]

- *BREAKING* Updates tx5 to a version using new endpoint state logic and a new incompatible protocol. [\#3287](https://github.com/holochain/holochain/pull/3287)
Copy link
Member

Choose a reason for hiding this comment

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

👍🏻 Is there going to be a back-port for this or is this just on 0.3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The plan right now is just on 0.3.

@@ -120,6 +121,7 @@ impl KitsuneTestHarness {
self.legacy_host_api.drain_events().await
}

#[allow(dead_code)]
Copy link
Member

Choose a reason for hiding this comment

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

Sorry yes, this is commented in the tests at the moment because they need more work. Thanks for cleaning up the warnings

Copy link
Contributor

@jost-s jost-s left a comment

Choose a reason for hiding this comment

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

Requesting to move sodoken dep to the workspace.

match evt {
tx5::EpEvt::Connected { rem_cli_url } => {
tx5::Ep3Event::Error(err) => {
panic!("Fatal tx5 error: {err:?}");
Copy link
Contributor

@jost-s jost-s Feb 6, 2024

Choose a reason for hiding this comment

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

While Holochain is becoming less network dependent, can Holochain continue functioning with local only data despite this failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not while it's a panic : )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just double checked the code, this can only happen in the case of a code mistake, so it is correct to fail fast and let us know something was written incorrectly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed my mind. I switched it to a tracing error after all: 63250c9

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, how come?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's possible to say a transient error will never in the future be introduced into this path... It would be a problem if some naive change in the future makes it so conductors crash every time a connection drops or something.

@neonphog
Copy link
Contributor Author

neonphog commented Feb 6, 2024

Requesting to move sodoken dep to the workspace.

@jost-s done: 8de1201

@neonphog neonphog requested a review from jost-s February 6, 2024 18:15
@neonphog neonphog enabled auto-merge (squash) February 6, 2024 21:26
@neonphog neonphog merged commit 473014c into develop Feb 6, 2024
11 checks passed
@neonphog neonphog deleted the integrate-tx5-ep3 branch February 6, 2024 22:20
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.

3 participants