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

Suggestion for ScriptContext changes in PlutusV4 #6408

Open
KtorZ opened this issue Aug 13, 2024 · 4 comments
Open

Suggestion for ScriptContext changes in PlutusV4 #6408

KtorZ opened this issue Aug 13, 2024 · 4 comments

Comments

@KtorZ
Copy link
Contributor

KtorZ commented Aug 13, 2024

Describe the feature you'd like

Hello, after playing around with Plutus V3 script context, I bumped into a few non-optimal things which I believe could be improved to avoid confusing situations and silly mistakes. Some of those points (e.g. 2) likely belong to intersectMBO/cardano-ledger but I am documenting them here since they are part of the same discussion, I reckon.

1 - Uniform Rational representations

We find rational numbers in two places in Plutus v3. The first one is inside the UpdateCommittee variant of GovernanceAction; which represents the new quorum for the next committee. Other places are inside the following protocol parameters: stake pool pledge influence, monetary expansion, treasury expansion, the new tier fee base for reference scripts and all the voting thresholds for dreps and spos (indices 9, 10, 11, 25, 26 and 33 respectively).

In the GovernanceAction, it is encoded as a Constr data, with index 0 and two data-integer fields. In other protocol parameters, they are encoded as a List data with 2 data-integer elements.

2 - Voter ordering in txInfoVotes

Votes are ordered by ascending voters, and then ascending governance action id. For multi-variant constructors (e.g. Voter), the ordering follows the order of the constructors ... except for Credential! For Credential, scripts are treated as lower values than pub key credentials. Or more specifically, the data received from the ledger will be ordered following this rule. The discrepancy here is seemingly due to the way the ledger encodes Credential (with the two variants swapped compared to the Plutus api):

https://github.com/IntersectMBO/cardano-ledger/blob/adb63e0c899109d89b5e99cc0d5b6a2e97fa3d2d/libs/cardano-ledger-core/src/Cardano/Ledger/Credential.hs#L81-L84

vs

data Credential
=
-- | The transaction that spends this output must be signed by the private key.
-- See `Crypto.PubKeyHash`.
PubKeyCredential PubKeyHash
-- | The transaction that spends this output must include the validator script and
-- be accepted by the validator. See `ScriptHash`.
| ScriptCredential ScriptHash
deriving stock (Eq, Ord, Show, Generic)
deriving anyclass (NFData)

Caution

This last point might lead to a bug in Plutus-Tx, which relies on the derived Ord instance for certificates in order to maintain a Map. I don't think this is currently a problem as I imagine the Map to be fully re-constructed when Scott-encoded; but that's perhaps something you may want to double-check?

3 - Segregate pointer stake credentials

This one isn't properly introduced in by Plutus v3, but I was somewhat hoping to see Pointer addresses gone from V3 contexts. Truth is, no one uses pointer addresses, and yet, they always get in the way when dealing with addresses. They keep confusing new developers, and they increase the costs of having to deal with stake credentials for everyone. My suggestion would be to introduce a second variant on Address into which move any pointer address:

data Address = 
    | Address
        { addressCredential        :: Credential -- ^ the payment credential
        , addressStakingCredential :: Maybe Credential -- ^ the staking credential
        }
    | PointerAddress 
        { addressCredential        :: Credential -- ^ the payment credential
        , addressStakingCredential :: (Int, Int, Int) -- ^ the staking credential
        }

Those not willing to deal with Pointer addresses can simply always ignore the second variant. And it makes working with standard addresses a bit more friendly (and efficient -- although I don't have precise numbers).

Describe alternatives you've considered

Not really applicable here.

@github-actions github-actions bot added the status: needs triage GH issues that requires triage label Aug 13, 2024
@zliu41
Copy link
Member

zliu41 commented Aug 13, 2024

cc @lehins
We'll revisit this with the ledger team once they begin development for Plutus V4, which is still several months away.

@lehins
Copy link

lehins commented Aug 13, 2024

@KtorZ Thank you for pointing out those inconsistencies. I wish we learned them about them sooner, but it'll teach us to pay a closer attention to the plutus context translation.

I was somewhat hoping to see Pointer addresses gone from V3 contexts. Truth is, no one uses pointer addresses, and yet, they always get in the way when dealing with addresses.

100% support you there. We have a multi step process of retiring those stupid pointer addresses. In Conway we disabled their resolution, so they can no longer contribute to stake distribution calculation. Once we are in Conway we'll be able to clean up a whole lot of code thanks to that. We can't really remove them completely because of older plutus scripts. We should have disabled pointer addresses for PlutusV3, but that somehow sneaked under the radar. So, we'll plan to fail PlutusV4 Context translation if pointer addresses are present in a transaction. This would likely require change to the Address type on the plutus side for PlutusV4, which should be a good thing, I believe.

As @zliu41 pointed out, we'll revisit this once we have a new era in place and when we get to implementing PlutusV4

@KtorZ
Copy link
Contributor Author

KtorZ commented Aug 13, 2024

I wish we learned them about them sooner

I wish I had gone through this sooner also. Work piled up and I am sadly only looking into this now.

we'll revisit this once we have a new era in place and when we get to implementing PlutusV4

Glad to hear! I hope I raised my concerns early enough this time 🤓. Too late for V3, but I hope in time for V4.

@colll78
Copy link
Contributor

colll78 commented Sep 2, 2024

cardano-foundation/CIPs#749
cardano-foundation/CIPs#735

I would also like to introduce:

  1. txInfoObservations :: [ScriptHash] as a field to the TxInfo in the ScriptContext
  2. Add new Maybe BuiltinData field to TxOut for output tagging / mechanism by which we can assocaite arbitrary data with transaction outputs

As described in the above CIPs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants