-
Notifications
You must be signed in to change notification settings - Fork 156
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
Rename TxBody and TxBodyRaw fields and patterns without underscores #3126
Conversation
01931b4
to
bc074c7
Compare
eras/shelley/test-suite/src/Test/Cardano/Ledger/Shelley/Address/Bootstrap.hs
Outdated
Show resolved
Hide resolved
bc074c7
to
21a6cb0
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.
There are a few accidental renames that weren't suppose to happen,other than that it looks pretty good
eras/shelley-ma/impl/src/Cardano/Ledger/ShelleyMA/Rules/Utxo.hs
Outdated
Show resolved
Hide resolved
eras/shelley-ma/impl/src/Cardano/Ledger/ShelleyMA/Rules/Utxo.hs
Outdated
Show resolved
Hide resolved
eras/shelley/test-suite/src/Test/Cardano/Ledger/Shelley/Generator/ShelleyEraGen.hs
Outdated
Show resolved
Hide resolved
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.
This is looking very good. I noticed some more stuff that can be improved upon the second review. It would also be nice to make pattern synonym record names for [Era]TxBody
constant for all eras as well. For example AlonzoTxBody
is defined as:
pattern AlonzoTxBody
{ inputs,
collateral,
outputs,
txcerts,
txwdrls,
txfee,
txvldt,
txUpdates,
reqSignerHashes,
mint,
scriptIntegrityHash,
adHash,
txnetworkid
}
But it should be something like this:
pattern AlonzoTxBody
{ atbInputs,
atbCollateral,
atbOutputs,
atbCerts,
atbWdrls,
atbTxFee,
atbValidityInterval,
atbUpdate,
atbReqSignerHashes,
atbMint,
atbScriptIntegrityHash,
atbAuxDataHash,
atbTxNetworkId
}
We want to make sure that there are not a dozen different names for a similar concept (eg txUpdates
vs atbrTxUpdate
or _txUpdate
prior to this PR - note that one is plural another one is not)
I guess the goal is to have no ambiguity between era and the least amount of discrepancy between record fields for the raw type, the actual newtype wrapper around it as well as the lens names for the corresponding fields.
e0ad49f
to
d563369
Compare
d563369
to
2655b72
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.
Sorry, more review comments. I am truly enjoying seeing this cleanup!
7e82cb2
to
a4113df
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 awesome. Thank you!
35239ad
to
e5856b8
Compare
Squashed and rebased! |
Can I merge this? |
@aniketd It's approved, good to merge I think! 🚀 |
go for it @aniketd ! |
Partially addresses #3061