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

Initial offline mode ADR #1012

Merged

Conversation

cardenaso11
Copy link
Contributor

@cardenaso11 cardenaso11 commented Aug 7, 2023

Making this draft publicly visible before the full PR etc, so everyone can see what to prepare for ahead of time. The current status of the implementation, is that it is nearing completion.

This ADR is for support for an offline mode, for the hydra-node target. SundaeSwap intends to contribute these patches upstream. Feedback is welcome, if there are any questions or suggestions or problems.


  • CHANGELOG updated or not needed
  • Documentation updated or not needed
  • Haddocks updated or not needed
  • No new TODOs introduced or explained hereafter

Copy link
Contributor

@abailly-iohk abailly-iohk left a comment

Choose a reason for hiding this comment

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

Some comments left, I think we could probably pair or work in team on this ADR to speed up the feedback loop

docs/adr/2023-07-05_024-offline-mode.md Outdated Show resolved Hide resolved
docs/adr/2023-07-05_024-offline-mode.md Outdated Show resolved Hide resolved
docs/adr/2023-07-05_024-offline-mode.md Outdated Show resolved Hide resolved
docs/adr/2023-07-05_024-offline-mode.md Outdated Show resolved Hide resolved
@cardenaso11
Copy link
Contributor Author

Some comments left, I think we could probably pair or work in team on this ADR to speed up the feedback loop

Thank you! Taking a look now. Making some initial adjustments based on feedback, will message about pairing. Sounds good to me

@abailly-iohk
Copy link
Contributor

Following our discussion, an updated lightweight architecture diagram taking into account what would be dropped/adapted:

ADRs _ Designs - Offline mode ADR

@abailly-iohk
Copy link
Contributor

@cardenaso11 Do you want more feedback on this? Is it ready for review, or do you want to make the ADR part of the PR introducing the feature?

@abailly-iohk
Copy link
Contributor

Follow-up discussion outcome:

  • Implement a "mock chain" by providing a Chain m tx handler and do not bother with mocking actual Cardano interactions
  • "Dependency injection" Is not really needed in Direct but could improve readability and maintainability of hydra-cluster code
  • Old mock-chain code from the early days of Hydra could be useful inspiration
    withMockChain :: ((Int, Int, Int) -> IO ()) -> IO ()
  • Polishing the off-line ADR
  • Need more work to refine the need for the event streaming ADR

@cardenaso11 cardenaso11 marked this pull request as ready for review October 6, 2023 19:03
@abailly-iohk abailly-iohk changed the title [DRAFT] Initial offline mode ADR Initial offline mode ADR Oct 9, 2023
@abailly-iohk
Copy link
Contributor

@cardenaso11 I would like to move this forward, I think the off-line ADR is in good shape and it's in Proposed state anyway, until the PR comes in. Can you remove the other one?

@cardenaso11
Copy link
Contributor Author

@cardenaso11 I would like to move this forward, I think the off-line ADR is in good shape and it's in Proposed state anyway, until the PR comes in. Can you remove the other one?

Absolutely, thanks a ton. I'd done that locally but think I had not pushed it

@cardenaso11 cardenaso11 force-pushed the offline-mode-adr branch 2 times, most recently from 1b09019 to 5635c00 Compare October 9, 2023 16:50
@cardenaso11
Copy link
Contributor Author

GitHub UI somehow rebased like 3 times instead of one for some reason, but either way, removed the other ADR. I also added the edits based on our meeting, let me know if anything else would help. Thanks a ton, @abailly-iohk

Copy link
Contributor

@abailly-iohk abailly-iohk left a comment

Choose a reason for hiding this comment

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

Some comments to clarify

docs/adr/2023-07-05_024-offline-mode.md Outdated Show resolved Hide resolved
docs/adr/2023-07-05_024-offline-mode.md Show resolved Hide resolved
docs/adr/2023-07-05_024-offline-mode.md Outdated Show resolved Hide resolved
docs/adr/2023-07-05_024-offline-mode.md Outdated Show resolved Hide resolved
docs/adr/2023-07-05_024-offline-mode.md Outdated Show resolved Hide resolved
docs/adr/2023-07-05_024-offline-mode.md Outdated Show resolved Hide resolved
Copy link
Contributor

@v0d1ch v0d1ch left a comment

Choose a reason for hiding this comment

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

@cardenaso11 This looks good to me. Not sure if we want to specify exactly how to get to the open Head with this initial utxo we will have as the argument to hydra-node and bypass Commit. Perhaps this would be too much information for an adr and can be left for the implementation.

docs/adr/2023-07-05_024-offline-mode.md Outdated Show resolved Hide resolved
docs/adr/2023-07-05_024-offline-mode.md Outdated Show resolved Hide resolved
@abailly-iohk
Copy link
Contributor

@cardenaso11 Apart from some minor comments, we are good to go :)

@cardenaso11
Copy link
Contributor Author

Great! Let me know if there's anything else remaining on this ADR to get merged, in the meantime I am gonna make sure the actual cleaned up implementation branch is reviewable for you guys by Monday. Thanks a ton!

@abailly-iohk
Copy link
Contributor

@cardenaso11 Seems like you need to add your handle to the authors.yml :)

add cardenaso11 (self) to authors.yaml
@abailly-iohk abailly-iohk merged commit ba04f39 into input-output-hk:master Oct 13, 2023
17 checks passed
@Quantumplation Quantumplation deleted the offline-mode-adr branch October 13, 2023 16:06
@abailly-iohk abailly-iohk mentioned this pull request Jan 17, 2024
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.

None yet

3 participants