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

Fix transTxCert always setting deposit to Nothing for TxCertRegStaking & TxCertUnRegStaking #4542

Conversation

KtorZ
Copy link
Contributor

@KtorZ KtorZ commented Aug 11, 2024

Doing some testing on my end, I noticed that deposits were always missing from TxCertRegStaking & TxCertUnRegStaking. However, they ought to be Just something when present; or more specifically when mapping a ConwayRegCert or ConwayUnRegCert to their Plutus counterpart.

The deposit should, in principle, only be set to zero when presented with a legacy certificate format. However, if we take the following certificate:

ConwayTxCertDeleg (ConwayRegCert
  (KeyHashObj (KeyHash {unKeyHash = "00000000000000000000000000000000000000000000000000000000"}))
  (SJust (Coin 3000000))
)

Using transTxCert we obtain:

TxCertRegStaking
  (PubKeyCredential 00000000000000000000000000000000000000000000000000000000)
  Nothing

And similarly for ConwayUnRegCert. This seems due to the order of clauses in the pattern match inside transTxCert which causes RegTxCert andUnRegTxCert to be matched even in the presence of a deposit. Swapping the pattern match clauses fixes the issue and return:

TxCertRegStaking
  (PubKeyCredential 00000000000000000000000000000000000000000000000000000000)
  (Just 3000000)

The writing of a regression test to capture this is left as an excercise for the maintainers 😬

Description

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated
  • All visible changes are prepended to the latest section of a CHANGELOG.md for the affected packages.
    New section is never added with the code changes. (See RELEASING.md)
  • When applicable, versions are updated in .cabal and CHANGELOG.md files according to the
    versioning process.
  • The version bounds in .cabal files for all affected packages are updated.
    If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)
  • Code is formatted with fourmolu (use scripts/fourmolize.sh)
  • Cabal files are formatted (use scripts/cabal-format.sh)
  • hie.yaml has been updated (use scripts/gen-hie.sh)
  • Self-reviewed the diff

…ertUnRegStaking

  Doing some testing on my end, I noticed that deposits where always missing
  from TxCertRegStaking & TxCertUnRegStaking. However, they ought to be `Just`
  something when present; or more specifically when mapping a `ConwayRegCert` or
  `ConwayUnRegCert` to their Plutus counterpart.

  The deposit should, in principle, only be set to zero when presented with a
  legacy certificate format. However, if we take the following certificate:

  ```hs
  ConwayTxCertDeleg (ConwayRegCert
    (KeyHashObj (KeyHash {unKeyHash = "00000000000000000000000000000000000000000000000000000000"}))
    (SJust (Coin 3000000))
  )
  ```

  Using `transTxCert` we obtain:

  ```
  TxCertRegStaking
    (PubKeyCredential 00000000000000000000000000000000000000000000000000000000)
    Nothing
  ```

  And similarly for `ConwayUnRegCert`. This seems due to the order of clauses in
  the pattern match inside `transTxCert` which causes `RegTxCert` and
  `UnRegTxCert` to be matched even in the presence of a deposit. Swapping the
  pattern match clauses fixes the issue and return:

  ```hs
  TxCertRegStaking
    (PubKeyCredential 00000000000000000000000000000000000000000000000000000000)
    (Just 3000000)
  ```

  The writing of a regression test to capture this is left as an excercise for
  the maintainers 😬
@KtorZ KtorZ changed the title Fix transTxCert setting deposit to Nothing for TxCertRegStaking & TxCertUnRegStaking Fix transTxCert always setting deposit to Nothing for TxCertRegStaking & TxCertUnRegStaking Aug 11, 2024
@lehins
Copy link
Collaborator

lehins commented Aug 12, 2024

Auch. That is an ugly bug. I guess we could fix it with an intra-era hard fork that should happen later on this year.
The actual bug is not in the TxInfo translation, but in the pattern synonym. these two guys should not have ignored the deposit field:

getRegTxCert (ConwayTxCertDeleg (ConwayRegCert c _)) = Just c
getRegTxCert _ = Nothing
mkUnRegTxCert c = ConwayTxCertDeleg $ ConwayUnRegCert c SNothing
getUnRegTxCert (ConwayTxCertDeleg (ConwayUnRegCert c _)) = Just c
getUnRegTxCert _ = Nothing

It should have been:

  getRegTxCert (ConwayTxCertDeleg (ConwayRegCert c SNothing)) = Just c
  getUnRegTxCert (ConwayTxCertDeleg (ConwayUnRegCert c SNothing)) = Just c

@lehins
Copy link
Collaborator

lehins commented Aug 27, 2024

Thank you for reporting this @KtorZ
We'll fix it properly at the intra-era hard fork. Here is the ticket for this bug: #4571

@lehins lehins closed this Aug 27, 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