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

Remove ServerOutput type and instead use StateChanged type #1800

Closed
Tracked by #1618
v0d1ch opened this issue Jan 27, 2025 · 1 comment
Closed
Tracked by #1618

Remove ServerOutput type and instead use StateChanged type #1800

v0d1ch opened this issue Jan 27, 2025 · 1 comment
Assignees
Labels
green 💚 Low complexity or well understood feature 💬 feature A feature on our roadmap

Comments

@v0d1ch
Copy link
Contributor

v0d1ch commented Jan 27, 2025

Why

Reduce boilerplate and have cleaner code. This is also a first step into achieving better memory profile for hydra-node #1618

What

  • Remove ServerOutput type and add/create constructors in StateChanged (Most ServerOutputs directly map to a StateChanged event, Some additional events need to be created to server outputs like PeerConnected et al)
  • Instead of cause ClientEffect, the HeadLogic would emit events with newState.
    Example:
    newState SnapshotConfirmed{snapshot, signatures = multisig}
    <> cause (ClientEffect $ ServerOutput.SnapshotConfirmed headId snapshot multisig)

    would become
    newState SnapshotConfirmed{headId, snapshot, signatures = multisig}
@v0d1ch v0d1ch added 💬 feature A feature on our roadmap green 💚 Low complexity or well understood feature labels Jan 27, 2025
@v0d1ch v0d1ch self-assigned this Jan 27, 2025
@noonio noonio moved this from Triage 🏥 to Todo 📋 in ☕ Hydra Team Work Jan 28, 2025
@v0d1ch v0d1ch moved this from Todo 📋 to In Progress 🕐 in ☕ Hydra Team Work Feb 4, 2025
@v0d1ch v0d1ch linked a pull request Feb 11, 2025 that will close this issue
4 tasks
@v0d1ch v0d1ch moved this from In progress 🕐 to In review 👀 in ☕ Hydra Team Work Feb 12, 2025
@ch1bo
Copy link
Member

ch1bo commented Feb 13, 2025

As I was reviewing the PR for this, I wondered.. why? The change described here seems to be a pure refactoring (no change in semantics) and I'm personally not convinced that this is exactly what we need.

Instead, we should be aiming to solve the actual problem #1618 and then do the refactoring based on the new semantics.

@noonio noonio closed this as not planned Won't fix, can't repro, duplicate, stale Feb 18, 2025
@github-project-automation github-project-automation bot moved this from In review 👀 to Done ✔ in ☕ Hydra Team Work Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
green 💚 Low complexity or well understood feature 💬 feature A feature on our roadmap
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants