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

Enable recovering in closed state too #1812

Open
v0d1ch opened this issue Jan 30, 2025 · 5 comments
Open

Enable recovering in closed state too #1812

v0d1ch opened this issue Jan 30, 2025 · 5 comments

Comments

@v0d1ch
Copy link
Contributor

v0d1ch commented Jan 30, 2025

Why

I ran into an issue where my funds got locked at the deposit contract after the Head got fanned-out. We could enable recovering also in the closed state so users have better time getting their funds back.

What

  • Propagate pendingDeposits map from the OpenState to ClosedState to enable the client input Recover working in both states.
  • Update HeadLogic/update function to enable recover in the Closed state too.
@v0d1ch v0d1ch self-assigned this Jan 30, 2025
@ch1bo
Copy link
Member

ch1bo commented Jan 30, 2025

How about keeping the pending deposits completely outside of the HeadState? AFAIK a deposit is not really tied to a specific head. Consequently the state is independent of whether a head is open or not. The API under which this is available could be an indicator too, it does not involve any head id, right? IIRC that would be GET /deposits and recovering a deposit is DELETE /deposits/<deposit-id>.

@v0d1ch
Copy link
Contributor Author

v0d1ch commented Jan 30, 2025

Where do you propose we keep this information? HeadLogic deals with HeadState only and has access to Environment also but that is not a proper place to put it either. New data type?

@v0d1ch v0d1ch removed their assignment Jan 30, 2025
@v0d1ch v0d1ch moved this from Triage 🏥 to Todo 📋 in ☕ Hydra Team Work Jan 30, 2025
@v0d1ch
Copy link
Contributor Author

v0d1ch commented Jan 30, 2025

Conversation from slack for the reference:

We drafted a notion of a NodeState in the past, which would contain one or more HeadState (even allowing multi-head node). So as the list of pending deposits is disconnected from any specific head (although they target a head, by id), we could think about keeping them there. So something like
data NodeState tx = NodeState { headState :: HeadState tx, pendingDeposits :: [Deposit tx], lastEventId :: Last EventId }

@noonio
Copy link
Contributor

noonio commented Jan 30, 2025

I like this; I'm tempted not to over-complicate it and just leave @ch1bo 's suggestion to future work; especially that I do think it's reasonable to consider deposit's basically directed at a specific head, as mentioned.

@ch1bo
Copy link
Member

ch1bo commented Jan 30, 2025

I like this; I'm tempted not to over-complicate it and just leave @ch1bo 's suggestion to future work; especially that I do think it's reasonable to consider deposit's basically directed at a specific head, as mentioned.

If we do not keep the deposits outside of HeadState I would argue that we need to add them to all variants of the sum type. Because the node could already see the head finalized or even adopted a new head altogether. If we make it convenient for users to list and recover deposits..it should be about all deposit seen, no matter which head we track.

An overall alternative would be to not provide that convenience at all and outsource the listing and recovering to something completely different. For example the explorer and the hydra-tx binary/library. This has other complexities though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo 📋
Development

No branches or pull requests

3 participants