Modify behaviour of valueAdjust

[sourabhxyz]

Currently, valueAdjust is a noop if an asset class is not present. valueAdjust (+1) GYLovelace mempty should be mempty or valueFromList [(GYLovelace, 1)]?

[TotallyNotChase]

Good catch. It should be the latter I think

[sourabhxyz]

Hey @TotallyNotChase, this issue was reported to us by a dev from MLabs, I just echoed it back here. Seeing the behaviour of underlying adjust function in Data.Map.Strict (which is too a noop if key is not present), I am inclined to think that perhaps we should leave it as it is and rather add some other utility to achieve similar purpose. I added valueAlter in this PR, would love to hear your thoughts on it.

[sourabhxyz]

Also, I am not sure if we should try maintaining the invariant of not allowing zero values in GYValue but it's worth noting that from Conway era onwards, zero values are not allowed at ledger level.

value = coin / [coin, multiasset<positive_coin>]

Here positive_coin ensures that value must be positive. Similarly, for mint, we have:

mint = multiasset<nonZeroInt64>

So values should not be zero.

[TotallyNotChase]

Also, I am not sure if we should try maintaining the invariant of not allowing zero values in GYValue but it's worth noting that from Conway era onwards, zero values are not allowed at ledger level.

value = coin / [coin, multiasset<positive_coin>]

Here positive_coin ensures that value must be positive. Similarly, for mint, we have:

mint = multiasset<nonZeroInt64>

So values should not be zero.

This sort of thing should really be handled in the section where app code is converted to ledger types. In fact, the ledger itself should be able to remove or add zeroes as necessary.

Absolutely insane that that is not the case in cardano-ledger

[sourabhxyz]

Absolutely insane that that is not the case in cardano-ledger

I should have also noted that cardano-ledger codebase actually does that. So even if we allow zero values, they are ultimately filtered for by cardano-ledger.

[TotallyNotChase]

That's perplexing. Then what does valueAdjust or valueAlter achieve? Does the ledger perform these normalizations only at certain contexts and not others? For example, is the same thing done for values put into datums?

Or is it just the fact that, the ledger is not aware of values in datums being... well... values. So when the ledger sees a datum serialized by some higher level framework, say atlas, that does not deal with these invariants - it just sort of.... lets it be...?

[sourabhxyz]

Does the ledger perform these normalizations only at certain contexts and not others

I think whenever we try to convert to underlying cardano-api/ledger value type, we see this normalisation (we use fromList to achieve this conversion). But if one is to put value into datum (could use valueToPlutus to get plutus-tx representation), we would bypass api/ledger conversion and would likely end up with zero entries in case GYValue also had them.

Or is it just the fact that, the ledger is not aware of values in datums being... well... values. So when the ledger sees a datum serialized by some higher level framework, say atlas, that does not deal with these invariants - it just sort of.... lets it be...?

I think so, yeah. But I am not sure if this was the original motivation to try filtering zero entries in GYValue. Fwiw, it was not originally present but was later added by a colleague but I just can't recall what was the reason, maybe it was just to simplify GYValue value as zero entries apparently aren't of use.

[t4ccer]

wrt. zero ADA entries it's fine to do whatever ledger expects, the problem is when converting GYValue to plutus-ledger-api Value. e.g. V1 and V2 TxInfo still has zero ADA entry in mint field, suppose you're convertingGYValue to Value and putting that in some datum, later validator checks if mint value equals that value. And sure, maybe validators should do smart value comparision but they don't, execution units are too limited and everybody is doing EqualsData instead, so these zero ADA entry matters.
Maybe even to the point where we'd need to have valueToPlutusWithZeroADA and valueToPlutusWithoutZeroADA and current one deprecated as it can lead to bugs

(this issue comes from my thread on slack)

[sourabhxyz]

Thanks for elaborating @t4ccer! Yes, I'm fine with your proposed change. Would you like to create a PR for it? Else we can handle it.

[sourabhxyz]

Alternatively, valueToPlutus could additionally accept PlutusVersion.