Skip to content

Conversation

@lehins
Copy link
Collaborator

@lehins lehins commented Oct 7, 2025

Description

Added an additional type parameter to Tx and TxBody to allow implementing multi-level transactions in Dijkstra.

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.

@Soupstraw Soupstraw force-pushed the lehins/multi-level-txs branch 2 times, most recently from f3fb62c to d18485a Compare October 20, 2025 14:46
@Soupstraw Soupstraw force-pushed the lehins/multi-level-txs branch 3 times, most recently from ea36171 to 2c62aed Compare October 22, 2025 12:11
@Soupstraw Soupstraw marked this pull request as ready for review October 22, 2025 12:13
@Soupstraw Soupstraw requested a review from a team as a code owner October 22, 2025 12:13
@lehins lehins mentioned this pull request Oct 22, 2025
10 tasks
@Soupstraw Soupstraw force-pushed the lehins/multi-level-txs branch 4 times, most recently from 8de443a to 7ca5d0d Compare October 23, 2025 12:51
Copy link
Collaborator Author

@lehins lehins left a comment

Choose a reason for hiding this comment

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

Looks great! Awesome work, @Soupstraw!

@Soupstraw Soupstraw force-pushed the lehins/multi-level-txs branch 6 times, most recently from c2becc7 to a0bcaa4 Compare October 27, 2025 16:26
Copy link
Contributor

@teodanciu teodanciu left a comment

Choose a reason for hiding this comment

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

Oh my god! This is a crazy PR! I reviewed until Alonzo, will do the rest tomorrow - sorry, my brain is hurting!
What's not clear to me is when to parameterize by the level type and when not , for example, ShelleyTx l era instead of ShelleyTx TopTx era, when defining instances (like: Show (ShelleyTx TopTx era)). Isn't it always gonna be ShelleyTx TopTx era ?
I understand for functions in rules, that they can get called with a different level parameter - but why is it useful for the types? Can we even have anything other than TopTx in ShelleyTx ?

Also mysterious I found that in some cases STxLevel l era ~ STxTopLevel l era was required to call mkSTxTopLevelM, and in others it seemed to know it.

I will look again tomorrow with fresh eyes and go through the rest of the changes.

@Soupstraw Soupstraw force-pushed the lehins/multi-level-txs branch 2 times, most recently from c449367 to c88793f Compare October 28, 2025 12:05
Copy link
Contributor

@teodanciu teodanciu 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 (albeit most overwhelming) to me.
Just some more small suggestions that can be ignored and some questions.

I'm in awe at both of you for making these big and complex changes work 🙇

@lehins lehins force-pushed the lehins/multi-level-txs branch 2 times, most recently from 8e44fbc to c4be317 Compare October 29, 2025 17:10
* Add `Cardano.Ledger.Core.TxLevel` module with level definitions

* Apply transaction level throught the ledegr codebase

* Add DijkstraSubTx

* Added DijkstraSubTxBody

Co-authored-by: Joosep Jääger <joosep.jaager@iohk.io>
@lehins lehins force-pushed the lehins/multi-level-txs branch from c4be317 to a7085d8 Compare October 29, 2025 18:42
@lehins lehins enabled auto-merge October 29, 2025 18:43
@lehins lehins merged commit 6321a57 into master Oct 29, 2025
120 of 122 checks passed
@lehins lehins deleted the lehins/multi-level-txs branch October 29, 2025 20:54
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.

4 participants