-
Notifications
You must be signed in to change notification settings - Fork 23
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
Simplify createTransactionBody
#333
Conversation
(txOuts bc) | ||
(txFee bc) | ||
(txWithdrawals bc) | ||
createTransactionBody sbe bc = |
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.
The focus of this PR is to simplify this function.
The earlier version of this code case matches on sbe
and handles each era therein separately resulting in a lot of duplicate code.
The new version of this function instead eliminates the large case expression and instead uses eons allow each feature to determine for themselves if they need to set their values into the TxBody.
createTransactionBody
6c5a15b
to
64936bc
Compare
createTransactionBody
createTransactionBody
caseShelleyToBabbageOrConwayEraOnwards | ||
(\w -> (L.apiUpdateTxBodyL w .~) <$> convTxUpdateProposal sbe (txUpdateProposal bc)) | ||
(const $ pure id) | ||
sbe |
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.
With this refactoring we can deal with each tx body feature in isolation without duplication.
This can be simplified further in a future PR.
& modifyWith setReqSignerHashes | ||
& modifyWith setReferenceInputs | ||
& modifyWith setCollateralReturn | ||
& modifyWith setTotalCollateral |
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.
modityWith
shouldn't be necessary but ghc
's type inference isn't quite there.
@@ -3025,27 +2933,28 @@ makeShelleyTransactionBody sbe@ShelleyBasedEraBabbage | |||
txMintValue, | |||
txScriptValidity | |||
} = do | |||
|
|||
let aOn = AllegraEraOnwardsBabbage |
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 just temporary because we need a witness. A future PR will get rid of this.
& L.totalCollateralTxBodyL .~ convTotalCollateral txTotalCollateral | ||
& L.certsTxBodyL .~ convCertificates sbe txCertificates | ||
& L.invalidBeforeTxBodyL aOn .~ convValidityLowerBound txValidityLowerBound | ||
& L.invalidHereAfterTxBodyL sbe .~ convValidityUpperBound sbe txValidityUpperBound |
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.
These lenses take witnesses instead of constraints. This can be used in future to simplify code.
|
||
in case sbe of | ||
ShelleyBasedEraShelley -> do |
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.
One of the goals of this PR is to eliminate to need to case match on the era and handle each era separately. Doing away with this case expression vastly reduces code duplication and the potential for error.
ec971ff
to
994f811
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.
994f811
to
ac1f2fd
Compare
setMint <- | ||
caseShelleyToAllegraOrMaryEraOnwards | ||
(const $ pure id) | ||
(const $ pure $ L.mintTxBodyL .~ convMintValue apiMintValue) | ||
sbe |
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.
Why do you need >>=
here? Why not:
setMint <- | |
caseShelleyToAllegraOrMaryEraOnwards | |
(const $ pure id) | |
(const $ pure $ L.mintTxBodyL .~ convMintValue apiMintValue) | |
sbe | |
let setMint = | |
caseShelleyToAllegraOrMaryEraOnwards | |
(const $ id) | |
(const $ L.mintTxBodyL .~ convMintValue apiMintValue) | |
sbe |
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 mostly to make all of the units of code have the same shape rather than have some unit use let
and others use <-
.
You are right in that the >>=
is not needed.
ac1f2fd
to
2aeb30b
Compare
Changelog
Context
Includes commits from #333 so review that first.
How to trust this PR
Highlight important bits of the PR that will make the review faster. If there are commands the reviewer can run to observe the new behavior, describe them.
Checklist