Skip to content

Add cardano open oracle protocol scripts for plutus-benchmark #7156

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
Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add cardano open oracle protocol scripts for plutus-benchmark #7156

wants to merge 5 commits into from

Conversation

SeungheonOh
Copy link
Collaborator

Implements Cardano Open Oracle Protocol scripts.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 18, 2025 •
edited
Loading

PR Preview Action v1.6.2

🚀 View preview at
https://IntersectMBO.github.io/plutus/pr-preview/pr-7156/

Built to branch gh-pages at 2025-07-04 15:42 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@SeungheonOh SeungheonOh force-pushed the seungheonoh/p1602-coop branch 4 times, most recently from 3ae1bc2 to 51947b9 Compare July 1, 2025 13:33
@SeungheonOh SeungheonOh self-assigned this Jul 1, 2025
@SeungheonOh SeungheonOh added the No Changelog Required Add this to skip the Changelog Check label Jul 1, 2025
@SeungheonOh SeungheonOh requested a review from Unisay July 1, 2025 13:33
SeungheonOh
SeungheonOh commented Jul 1, 2025
View reviewed changes
plutus-core/executables/src/PlutusCore/Executable/AstIO.hs
NamedDeBruijn -> BSL.fromStrict . flat .UPLC.UnrestrictedProgram . toNamedDeBruijnUPLC
Named -> BSL.fromStrict . flat . UPLC.UnrestrictedProgram
DeBruijn -> BSL.fromStrict . flat . UPLC.UnrestrictedProgram . toDeBruijnUPLC
NamedDeBruijn -> BSL.fromStrict . flat . UPLC.UnrestrictedProgram . toNamedDeBruijnUPLC
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wasn't able to resist here

@SeungheonOh SeungheonOh force-pushed the seungheonoh/p1602-coop branch from 51947b9 to 2998015 Compare July 1, 2025 13:36
@SeungheonOh SeungheonOh force-pushed the seungheonoh/p1602-coop branch from 2998015 to df25a6e Compare July 1, 2025 13:37
SeungheonOh
SeungheonOh commented Jul 1, 2025
View reviewed changes
plutus-benchmark/coop/src/PlutusBenchmark/Coop/Scripts.hs
certMp :: CompiledCode (CertMpParams -> BuiltinData -> BuiltinData -> BuiltinUnit)
certMp = $$(compile [|| \p r sc -> certMp' p (unsafeFromBuiltinData r) (unsafeFromBuiltinData sc) ||])

fsMp' :: FsMpParams -> FsMpRedeemer -> ScriptContext -> BuiltinUnit
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm aware there's Data backed versions, which I found out later. Sadly, pattern synonym doesn't work with wildcards.

We need to have discussion around this. I think we should just have arbitrary datatype to have configurable underlying representation, which allows other optimizations as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remember to bring up this topic at the next technical discussion meeting!

@SeungheonOh SeungheonOh force-pushed the seungheonoh/p1602-coop branch from dab1baa to 5194009 Compare July 1, 2025 19:17
Unisay
Unisay reviewed Jul 3, 2025
View reviewed changes
@SeungheonOh SeungheonOh force-pushed the seungheonoh/p1602-coop branch from 5194009 to 8c9964f Compare July 3, 2025 15:56
Unisay
Unisay reviewed Jul 4, 2025
View reviewed changes
Copy link
Contributor

@Unisay Unisay left a comment

Choose a reason for hiding this comment

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

Thanks for the explicit imports!

I was expecting all types from the Plutus Ledger API to be imported from the PlutusLedgerApi.V3 namespace (or the PlutusLedgerApi.Data.V3). Could you verify this is the case, please?

plutus-benchmark/coop/src/PlutusBenchmark/Coop/Utils.hs
import PlutusLedgerApi.V2
import PlutusTx.AssocMap
import PlutusLedgerApi.V1.Value (Value (Value), flattenValue, valueOf, withCurrencySymbol)
import PlutusLedgerApi.V2 (CurrencySymbol, Datum (Datum), DatumHash,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why V2 not V3 ?

@SeungheonOh
Copy link
Collaborator Author

The original scripts were V2 scripts; I replicated original script exactly for both ease of testing and following code

@Unisay
Copy link
Contributor

Unisay commented Jul 10, 2025

The original scripts were V2 scripts;

It could be that we've improved something in V3 so I see no point in bringing something potentially sub-optimal.

I replicated original script exactly for both ease of testing and following code

Sorry, I don't understand how using V2 makes it easier to test or follow 🤷🏼‍♂️

Unisay
Unisay reviewed Jul 10, 2025
View reviewed changes
Unisay
Unisay reviewed Jul 10, 2025
View reviewed changes
plutus-ledger-api/src/PlutusLedgerApi/V3/MintValue.hs Outdated
@@ -20,6 +20,7 @@ module PlutusLedgerApi.V3.MintValue
( MintValue (..) -- Constructor is exported for testing
, emptyMintValue
, mintValueToMap
, mintValueToValue
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
, mintValueToValue

@SeungheonOh
Copy link
Collaborator Author

V2 makes comparison more effective in this case because of two reasons mainly:

  • Script context deconstruction cost can be differ between different plutus ledger api versions. For example, deconstructing script context in V1 context is often always more expensive than doing it for V2 even with data because they have a bunch of types like CurrencySymbol or PublicKeyHash wrapped in Constr 0. So when comparing costs between two implementations, it's better to keep ledger api version same for a pair comparison.
  • Secondly, It's much easier more so in this case because we have vendor provided generators for script contexts that will generate V2 script context. Having two script that shares same input ledger api format would be simple to do.

@Unisay
Copy link
Contributor

Unisay commented Jul 10, 2025

  • Script context deconstruction cost can be differ between different plutus ledger api versions. For example, deconstructing script context in V1 context is often always more expensive than doing it for V2 even with data because they have a bunch of types like CurrencySymbol or PublicKeyHash wrapped in Constr 0. So when comparing costs between two implementations, it's better to keep ledger api version same for a pair comparison.

What is it you're comparing it against?

@SeungheonOh
Copy link
Collaborator Author

The plutarch implementation: https://github.com/mlabs-haskell/cardano-open-oracle-protocol

@SeungheonOh
Copy link
Collaborator Author

SeungheonOh commented Jul 10, 2025 •
edited
Loading

@zliu41 Do we want this comparison against plutarch to be inside of plutus-benchmark. If not, I'm just going to run it separately and report numbers here. Since Plutarch implementation won't change, we just need to run the numbers for plutarch scripts once

@Unisay
Copy link
Contributor

Unisay commented Jul 11, 2025

@zliu41 Do we want this comparison against plutarch to be inside of plutus-benchmark. If not, I'm just going to run it separately and report numbers here. Since Plutarch implementation won't change, we just need to run the numbers for plutarch scripts once

I have a task to collect and write down such requirements, its in progress. For now I'd say it is enough to do a minimal thing, as we haven't agreed yet on what the maximal thing is going to be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Unisay Unisay Unisay left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@Unisay Unisay

@SeungheonOh SeungheonOh

Labels
No Changelog Required Add this to skip the Changelog Check
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SeungheonOh @Unisay