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.

Sign up for GitHub

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

Jump to bottom

Unwrap script validity field in TxBodyContent #3091

Merged
merged 1 commit into from
Aug 22, 2021
Merged

Unwrap script validity field in TxBodyContent #3091

merged 1 commit into from
Aug 22, 2021

Conversation

newhoggy
Copy link
Contributor

No description provided.

Jimbo4350
Jimbo4350 approved these changes Aug 18, 2021
View reviewed changes
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

LGTM! Maybe let Duncan have a look as well.

dcoutts
dcoutts previously requested changes Aug 18, 2021
View reviewed changes
Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

Good so far, but we have to plumb the flag through to the right place so we can properly have the flag value go back and forth with the underlying ledger tx type (which does contain the flag).

type instance Core.Tx (AlonzoEra c) = ValidatedTx (AlonzoEra c)

data ValidatedTx era = ValidatedTx
  { body :: !(Core.TxBody era),
    wits :: !(TxWitness era),
    isValidating :: !IsValidating,
    auxiliaryData :: !(StrictMaybe (Core.AuxiliaryData era))
  }

So as you see, the flag value is directly available.

cardano-api/src/Cardano/Api/TxBody.hs Outdated
Comment on lines 1596 to 1602
pattern TxBody txbodycontent <- (getTxBodyContent TxScriptValidityNone -> txbodycontent)
{-# COMPLETE TxBody #-}

getTxBodyContent :: TxBody era -> TxBodyContent ViewTx era
getTxBodyContent (ByronTxBody body) = getByronTxBodyContent body
getTxBodyContent (ShelleyTxBody era body _scripts _redeemers mAux _scriptValidity) =
fromLedgerTxBody era body mAux
getTxBodyContent :: TxScriptValidity era -> TxBody era -> TxBodyContent ViewTx era
getTxBodyContent _ (ByronTxBody body) = getByronTxBodyContent body
getTxBodyContent scriptValidity (ShelleyTxBody era body _scripts _redeemers mAux _scriptValidity) =
fromLedgerTxBody era scriptValidity body mAux
Copy link
Contributor

Choose a reason for hiding this comment

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

No we would have to add the validity flag into the TxBody type, probably as an extra field in the ShelleyTxBody constructor. We have to get the value from the underlying ledger tx type, and not invent it. For one thing we need to do that to have the TxBody round trip test pass, but more importantly we need it to get the right value for the tx from a block.

Copy link
Contributor

Choose a reason for hiding this comment

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

See also getAlonzoTxBody where we extract the parts of the Alonzo.ValidatedTx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remove the argument of getTxBodyContent and am taking it from TxBody.

@newhoggy newhoggy force-pushed the unwrap-script-validity-in-tx-body-content branch 2 times, most recently from 231f435 to 95e5452 Compare August 19, 2021 11:53
@newhoggy newhoggy requested a review from dcoutts August 19, 2021 11:55
@newhoggy newhoggy dismissed dcoutts’s stale review August 19, 2021 12:11

Comments addressed

@newhoggy
Copy link
Contributor Author

bors merge

iohk-bors bot added a commit that referenced this pull request Aug 20, 2021
@iohk-bors @newhoggy
Merge #3091
13ac8e7 
3091: Unwrap script validity field in TxBodyContent r=newhoggy a=newhoggy



Co-authored-by: John Ky <john.ky@iohk.io>
@newhoggy newhoggy force-pushed the unwrap-script-validity-in-tx-body-content branch from 95e5452 to 528a573 Compare August 20, 2021 00:54
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 20, 2021

Canceled.

@newhoggy newhoggy force-pushed the unwrap-script-validity-in-tx-body-content branch 2 times, most recently from e5efa5a to 31e6c75 Compare August 20, 2021 04:37
MarcFontaine
MarcFontaine approved these changes Aug 20, 2021
View reviewed changes
Copy link
Contributor

@MarcFontaine MarcFontaine left a comment

Choose a reason for hiding this comment

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

LGTM

@newhoggy newhoggy force-pushed the unwrap-script-validity-in-tx-body-content branch from 31e6c75 to 80cf2f1 Compare August 21, 2021 05:16
@newhoggy
Copy link
Contributor Author

bors merge

iohk-bors bot added a commit that referenced this pull request Aug 21, 2021
@iohk-bors @newhoggy
Merge #3091
9033ace 
3091: Unwrap script validity field in TxBodyContent r=newhoggy a=newhoggy



Co-authored-by: John Ky <john.ky@iohk.io>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 21, 2021

Build failed:

@newhoggy newhoggy force-pushed the unwrap-script-validity-in-tx-body-content branch from 80cf2f1 to 45ce7fd Compare August 22, 2021 07:13
@newhoggy
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request Aug 22, 2021
@iohk-bors @newhoggy
Merge #3091
e3cf19c 
3091: Unwrap script validity field in TxBodyContent r=newhoggy a=newhoggy



Co-authored-by: John Ky <john.ky@iohk.io>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 22, 2021

Build failed:

@johnalotoski
Copy link
Contributor

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 22, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit a52f20e into master Aug 22, 2021
@iohk-bors iohk-bors bot deleted the unwrap-script-validity-in-tx-body-content branch August 22, 2021 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erikd erikd erikd approved these changes

@MarcFontaine MarcFontaine MarcFontaine approved these changes

@Jimbo4350 Jimbo4350 Jimbo4350 approved these changes

@DavidEichmann DavidEichmann Awaiting requested review from DavidEichmann

@deepfire deepfire Awaiting requested review from deepfire

@denisshevchenko denisshevchenko Awaiting requested review from denisshevchenko

@jutaro jutaro Awaiting requested review from jutaro

@dcoutts dcoutts Awaiting requested review from dcoutts

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@newhoggy @johnalotoski @erikd @dcoutts @MarcFontaine @Jimbo4350