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.

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

Seungheonoh/moveledger #805

Merged
merged 25 commits into from
Jan 29, 2025
Merged

Seungheonoh/moveledger #805

merged 25 commits into from
Jan 29, 2025

Conversation

SeungheonOh
Copy link
Collaborator

@SeungheonOh SeungheonOh commented Jan 25, 2025

Redo ledger types to use new derivation strategies.
Remove PTryFrom

@SeungheonOh SeungheonOh changed the base branch from master to staging January 25, 2025 06:24
)
~ ()

class (PNormalIsData' a (PInnest a), PInnest a ~ PData) => PNormalIsData a
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

any idea for better name?

Copy link
Contributor

Choose a reason for hiding this comment

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

PInnermostIsData?

@SeungheonOh
Copy link
Collaborator Author

New structure can be somewhat painful if we want to access nested structures. like foo.resolved.address or something like that. Optics would be a good option to have some short cuts.

Plutarch/Internal/Term.hs Outdated Show resolved Hide resolved
Plutarch/Repr/Data.hs Outdated Show resolved Hide resolved
)
~ ()

class (PNormalIsData' a (PInnest a), PInnest a ~ PData) => PNormalIsData a
Copy link
Contributor

Choose a reason for hiding this comment

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

PInnermostIsData?

Copy link
Collaborator

@t4ccer t4ccer left a comment

Choose a reason for hiding this comment

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

Didn't read too much into internals, do we still get that magical optimization that extracting multiple record fields traverses the list only once?

findPlaceholder (RPlaceHolder idx') = idx == idx'
findPlaceholder (RConstr _ xs) = any findPlaceholder xs
findPlaceholder (RCase x xs) = any findPlaceholder (x : xs)
findPlaceholder _ = False
Copy link
Collaborator

@t4ccer t4ccer Jan 27, 2025

Choose a reason for hiding this comment

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

Wildcard _ is an anti-pattern in AST traversals like this, please name constructors

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this.

Comment on lines 21 to +30
data POutputDatum (s :: S)
= PNoOutputDatum (Term s (PDataRecord '[]))
| POutputDatumHash (Term s (PDataRecord '["datumHash" ':= PDatumHash]))
= PNoOutputDatum
| POutputDatumHash
{ poutputDatum'datumHash :: Term s (PAsData PDatumHash)
}
| -- | Inline datum as per
-- [CIP-0032](https://github.com/cardano-foundation/CIPs/blob/master/CIP-0032/README.md)
POutputDatum (Term s (PDataRecord '["outputDatum" ':= PDatum]))
POutputDatum
{ poutputDatum'outputDatum :: Term s PDatum
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

That generates partial record selectors, not a huge fan

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the future, we should have better interface to each fields using optics. I want to let it slip for release at this point

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should just remove them and have "unnamed sop" (whatever the name is), e.g. PScriptPurpose is already doing that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

names follow from definitions of PlutusLedgerApi. I think it's better to have this.

It's less of a problem for us because this explodes when Haskell gets compiled and not on UPLC result.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh right, this will straight up always error during plutarch compilation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I don't worry about it too much because of it

]
]
]
-- , plutarchGolden
Copy link
Collaborator

Choose a reason for hiding this comment

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

All of that commented out code in testlib should go

@SeungheonOh
Copy link
Collaborator Author

SeungheonOh commented Jan 29, 2025

Didn't read too much into internals, do we still get that magical optimization that extracting multiple record fields traverses the list only once?

Yes, and it does this without any typelevel stuff. pmatch looks ahead and kills any unused branches

@SeungheonOh SeungheonOh force-pushed the seungheonoh/moveledger branch from 372337c to a97cb93 Compare January 29, 2025 01:26
@SeungheonOh SeungheonOh force-pushed the seungheonoh/moveledger branch from a97cb93 to cf5fdfe Compare January 29, 2025 01:37
@SeungheonOh SeungheonOh merged commit 8fd8780 into staging Jan 29, 2025
2 checks passed
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.

3 participants