Skip to content

Conversation

ana-pantilie
Copy link
Contributor

Copy link
Contributor

github-actions bot commented Jul 21, 2025

PR Preview Action v1.6.2

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

Built to branch gh-pages at 2025-09-05 16:28 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@zliu41 zliu41 marked this pull request as ready for review September 3, 2025 23:35
@zliu41 zliu41 added the No Changelog Required Add this to skip the Changelog Check label Sep 3, 2025
@zliu41 zliu41 requested review from kwxm, effectfully, SeungheonOh and a team September 3, 2025 23:40
@zliu41
Copy link
Member

zliu41 commented Sep 3, 2025

Ready for review. This is a naive implementation, where Value is simply a newtype wrapper around Map ByteString (Map ByteString Integer). The problem is that this doesn't allow efficient access to the size of the largest inner map - it takes O(m) time to do so where m is the size of the outer map. This is likely a problem for costing operations like insertCoin and deleteCoin, which only take O(logm) + O(logk), where k is the size of the largest inner map, since size lookup is more expensive than the operations themselves.

Nonetheless wen can merge this first, and if we need a different representation of Value we can modify it in a separate PR.

Copy link
Collaborator

@SeungheonOh SeungheonOh left a comment

Choose a reason for hiding this comment

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

Everything looks reasonable besides some nitpicks.

conValue =
Value.fromList
<$> conList
(DefaultUniPair
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can probably use some better syntax for value. Just using 3tuple for each entry would be more readable.

Copy link
Member

Choose a reason for hiding this comment

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

That may be slightly better-looking, but then we won't be able to reuse the built-in list parser since a built-in list can't contain a 3-tuple. I'll leave this as future work.

(DefaultUniPair DefaultUniByteString DefaultUniInteger)
)
)
conValue = Value.fromList <$> conList knownUni
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain this change? I remember I wasn't sure about the former implementation, so I'd like to understand how this all works.

Copy link
Member

Choose a reason for hiding this comment

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

knownUni :: uni `Contains` a => uni (Esc a), so it can be instantiated to any uni (Esc a) as long as uni `Contains` a. So there's no need to specify the concrete type.

@@ -118,6 +119,7 @@ data DefaultUni a where
DefaultUniBLS12_381_G1_Element :: DefaultUni (Esc BLS12_381.G1.Element)
DefaultUniBLS12_381_G2_Element :: DefaultUni (Esc BLS12_381.G2.Element)
DefaultUniBLS12_381_MlResult :: DefaultUni (Esc BLS12_381.Pairing.MlResult)
DefaultUniValue :: DefaultUni (Esc Value)
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we agreed to call it MaryValue? Let's not repeat ledger folks' mistakes
image

Copy link
Member

Choose a reason for hiding this comment

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

I don't recall the discussion - do you have a pointer? It's always called Value in plutus, and I'm hesitant to make a name 80% longer unless there's a clear advantage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No Changelog Required Add this to skip the Changelog Check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants