-
Notifications
You must be signed in to change notification settings - Fork 166
Multi level transaction definition #5334
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
Conversation
8a37d8c to
90fa0f2
Compare
a0b4161 to
a435e9b
Compare
f3fb62c to
d18485a
Compare
ea36171 to
2c62aed
Compare
8de443a to
7ca5d0d
Compare
There was a problem hiding this 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!
eras/dijkstra/impl/testlib/Test/Cardano/Ledger/Dijkstra/TreeDiff.hs
Outdated
Show resolved
Hide resolved
c2becc7 to
a0bcaa4
Compare
There was a problem hiding this 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.
c449367 to
c88793f
Compare
There was a problem hiding this 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 🙇
eras/dijkstra/impl/testlib/Test/Cardano/Ledger/Dijkstra/TreeDiff.hs
Outdated
Show resolved
Hide resolved
8e44fbc to
c4be317
Compare
* 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>
c4be317 to
a7085d8
Compare
Description
Added an additional type parameter to
TxandTxBodyto allow implementing multi-level transactions inDijkstra.Checklist
CHANGELOG.mdfiles updated for packages with externally visible changes.NOTE: New section is never added with the code changes. (See RELEASING.md).
.cabalandCHANGELOG.mdfiles when necessary, according to theversioning process.
.cabalfiles updated when necessary.NOTE: If bounds change in a cabal file, that package itself must have a version increase. (See RELEASING.md).
scripts/fourmolize.sh).scripts/cabal-format.sh).scripts/gen-cddl.sh)hie.yamlupdated (usescripts/gen-hie.sh).