Shelley: Move withdrawals draining from DELEGS to LEDGER

[aniketd]

Description

Checklist

• Commits in meaningful sequence and with useful messages.
• Tests added or updated when needed.
• CHANGELOG.md files updated for packages with externally visible changes.
  NOTE: New section is never added with the code changes. (See RELEASING.md).
• Versions updated in .cabal and CHANGELOG.md files when necessary, according to the
  versioning process.
• Version bounds in .cabal files updated when necessary.
  NOTE: If bounds change in a cabal file, that package itself must have a version increase. (See RELEASING.md).
• Code formatted (use scripts/fourmolize.sh).
• Cabal files formatted (use scripts/cabal-format.sh).
• CDDL files are up to date (use scripts/gen-cddl.sh)
• hie.yaml updated (use scripts/gen-hie.sh).
• Self-reviewed the diff.

[neilmayhew]

I assume there should be a CDDL change that goes along with this.

[neilmayhew]

The indentation seems off here

[lehins]

Looks like a problem with fourmolu or ormolu. Wanna create a minimal reproducer, see who is at fault and submit a ticket about it?

[aniketd]

I noticed the same indentation in govTransition and conwayDelegTransition and other places, seemingly consistent. Will definitely create a minimal reproducer 👍 and open an issue, though I'm not sure as to what the expected indentation is and what is exactly wrong with this one.

[aniketd]

@neilmayhew

I assume there should be a CDDL change that goes along with this.

Although PredicateFailures have CBOR instances I don't think we maintain a .cddl specification for them. Please correct me if I'm wrong.

[lehins]

Although PredicateFailures have CBOR instances I don't think we maintain a .cddl specification for them.

We have a long standing issue about that: #3392

One day we will implement it. Maybe we can start with Dijkstra era. I think that would be a huge step forward in terms of downstream usability

[lehins]

I'm not sure as to what the expected indentation is and what is exactly wrong with this one.

@aniketd I'd suggest look for existing issues about that on ormolu and fourmolu and if there is nothing there, then starting a discussion about this won't hurt. I suspect there might be a reason why it is done that way. I can certainly see why, for example, it has to be done that way in the do syntax, when pattern matching on just a tuple:

foo = do
 ( a
   , b
   , c ) <- blah
 pure c

Because if you don't, then it will result in an invalid syntax:

foo = do
 ( a
 , b
 , c ) <- blah
 pure c

But, this is not a problem when it is with another type like here TRC (a, ....

[teodanciu]

Looks good to me, but some tests are failing, they probably need to be adjusted to the changes.

[teodanciu]

This is duplicated in Conway but I can't come up with any meaningful name for what it's doing, since it's just calling this function and populating the Maybes, so I don't think there's any point extracting and reusing it.
Including it in the function is also not an option, since the resulting Maybe is used on its own on the other branch of the HF check.
So anyway, the duplication is a bit unpleasant, but i don't have any solution, just pointing it out.

[lehins]

There is a very nice way to deduplicate this. Predicate injection mechanism allows us to extract both of the checks and reuse them directly in Conway too:

testIncompleteAndMissingWithdrawals = do
  network <- liftSTS $ asks networkId
  let accounts = certState ^. certDStateL . accountsL
      withdrawals = tx ^. bodyTxL . withdrawalsTxBodyL
      (invalidWithdrawals, incompleteWithdrawals) =
        case withdrawalsThatDoNotDrainAccounts withdrawals network accounts of
          Nothing -> (Nothing, Nothing)
          Just (invalid, incomplete) ->
            ( if null (unWithdrawals invalid) then Nothing else Just invalid
            , if null (unWithdrawals incomplete) then Nothing else Just incomplete
            )
  failOnJust invalidWithdrawals (injectFailure . ShelleyWithdrawalsMissingAccounts)
  failOnJust incompleteWithdrawals (injectFailure . ShelleyIncompleteWithdrawals)

[aniketd]

I was able to get this to work by passing the constructors for the predicate failures as arguments, since the constructors themselves are era specific. The other way I see this can be done is by keeping the failOnJust lines as they are (within the rule transition functions) and making testIncompleteAndMissingWithdrawals return the missing or incomplete withdrawals, perhaps also changing its name to something like getIncompleteAndMissingWithdrawals. What do you think?

[lehins]

What do you think?

I think you did not look into my suggestion very carefully. The key part of the suggestion is injectFailure.

[lehins]

Looks great! Once duplication is removed as suggestd we should be able to merge this PR.
Thank you!

[lehins]

Looks like a problem with fourmolu or ormolu. Wanna create a minimal reproducer, see who is at fault and submit a ticket about it?

[lehins]

There is a very nice way to deduplicate this. Predicate injection mechanism allows us to extract both of the checks and reuse them directly in Conway too:

testIncompleteAndMissingWithdrawals = do
  network <- liftSTS $ asks networkId
  let accounts = certState ^. certDStateL . accountsL
      withdrawals = tx ^. bodyTxL . withdrawalsTxBodyL
      (invalidWithdrawals, incompleteWithdrawals) =
        case withdrawalsThatDoNotDrainAccounts withdrawals network accounts of
          Nothing -> (Nothing, Nothing)
          Just (invalid, incomplete) ->
            ( if null (unWithdrawals invalid) then Nothing else Just invalid
            , if null (unWithdrawals incomplete) then Nothing else Just incomplete
            )
  failOnJust invalidWithdrawals (injectFailure . ShelleyWithdrawalsMissingAccounts)
  failOnJust incompleteWithdrawals (injectFailure . ShelleyIncompleteWithdrawals)