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

foldClient with ledger state #135

Conversation

the-headless-ghost
Copy link
Contributor

No description provided.

@the-headless-ghost the-headless-ghost marked this pull request as ready for review April 4, 2024 14:31
Copy link
Owner

@j-mueller j-mueller left a comment

Choose a reason for hiding this comment

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

I think we need two variants of foldClient' - one that keeps track of the ledger state, and one that doesn't. It should be possible for users of the library to opt out of keeping k versions of the entire ledger state in memory.

Maybe we could add some parameters like this

-- | Whether to keep track of the full ledger state on the client
data LedgerStateMode = FullLedgerState | NoLedgerState

data LedgerStateArgs mode where
  NoLedgerStateArgs :: LedgerStateArgs 'NoLedgerState
  LedgerStateArgs :: LedgerState -> LedgerStateArgs 'FullLedgerState

data LedgerStateUpdate mode where
  NoLedgerStateUpdate :: LedgerStateUpdate 'NoLedgerState
  LedgerStateUpdate :: LedgerState -> [LedgerEvent] -> LedgerStateUpdate 'FullLedgerState


foldClient' ::
  forall s w mode.
  Monoid w =>
  s -- ^ Initial state
  -> LedgerStateArgs mode -- ^ Initial ledger state arguments
  -> Env -- ^ Node connection data
  -> (ChainPoint -> w -> s -> IO (w, s)) -- ^ Rollback
  -> (CatchingUp -> s -> LedgerStateUpdate mode -> BlockInMode CardanoMode -> IO (Maybe (w, s))) -- ^ Fold
  -> PipelinedLedgerStateClient

What do you think?

We could even add a field for the validation mode in LedgerStateArgs (to resolve the TODO in l. 181)

newState <- accumulate
cu
currentState
currentLedgerState
Copy link
Owner

Choose a reason for hiding this comment

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

Here we're passing the old ledger state to accumulate, but in foldBlocks the accumulator gets the new ledger state, with the block applied. Is this intentional? (same for currentLedgerEvents)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right!

env
(\_ _ !s -> pure ((), s))
(\c !s -> fmap (fmap pure) . applyBlock c s)
(\c !s ls le -> fmap (fmap pure) . accumulate c s ls le)
Copy link
Owner

Choose a reason for hiding this comment

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

I believe there should be a ! on ls

Copy link
Owner

@j-mueller j-mueller left a comment

Choose a reason for hiding this comment

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

Just missing some comments now

@@ -65,6 +74,20 @@ data CatchingUp =
deriving stock (Eq, Show, Generic)
deriving anyclass (FromJSON, ToJSON)

data LedgerStateMode = FullLedgerState | NoLedgerState
Copy link
Owner

Choose a reason for hiding this comment

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

Some haddocks explaining what this does (switching between ledger state / no ledger state) would be nice

Copy link
Owner

@j-mueller j-mueller left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@the-headless-ghost
Copy link
Contributor Author

Thanks for the review!

@j-mueller j-mueller merged commit e3b1230 into j-mueller:node-1.35.4 Apr 8, 2024
4 checks passed
@j-mueller
Copy link
Owner

@the-headless-ghost do you feel up to porting this to the main branch too?

the-headless-ghost added a commit to the-headless-ghost/sc-tools that referenced this pull request Apr 18, 2024
j-mueller pushed a commit that referenced this pull request Apr 22, 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.

2 participants