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

Please expose internal variants that are not safety opinionated #735

Open
colll78 opened this issue Oct 30, 2024 · 4 comments
Open

Please expose internal variants that are not safety opinionated #735

colll78 opened this issue Oct 30, 2024 · 4 comments

Comments

@colll78
Copy link

colll78 commented Oct 30, 2024

https://github.com/Plutonomicon/plutarch-plutus/blob/93833546d61d7a463d3ec25e8fbd16ba1abd0ec1/Plutarch/ByteString.hs#L240C1-L291C43

The unique value proposition of Plutarch over other languages targeting PlutusCore is that it is not opinionated. It gives the developer full low-level control over everything from strictness to inlining vs hoisting to binding and so forth. This is important because if you are very knowledgeable in the domain of Cardano smart contracts, you can write extremely efficient code that you cannot write in the other languages where the compilers are more opinionated (ie the compiler makes choices on your behalf regarding strictness or whether something is inlined or not, etc).

Please don't include any opinioned logic in builtin internal functions, if you truly want such logic please include it a builtin external module so that developers who don't want that logic can still use the internal module (which I expect is at-least 95% of the developers using Plutarch since it is a hyper-efficiency focused DSL).

For example,

preplicateBS :: forall (s :: S). Term s (PInteger :--> PByte :--> PByteString)
preplicateBS = phoistAcyclic $ plam go
  where
    go :: forall (s' :: S). Term s' PInteger -> Term s' PByte -> Term s' PByteString
    go len w8 =
      punsafeBuiltin PLC.ReplicateByte
        # pif (len #< 0) 0 len
        # punsafeCoerce @_ @_ @PInteger w8
        
pzeroesBS :: forall (s :: S). Term s (PInteger :--> PByteString)
pzeroesBS = ...

ponesBS = ...

All of these functions have this pif (len #< 0) 0 len to check that the value is non-negative. There is no reason for this check. Optional types and so forth for delayed failure are an anti-pattern in onchain code, why would you ever want to pass this a negative integer and not have it error? I'd imagine this might even cause some vulnerabilities if the caller is not expecting negative integers to be passed. Even if you do want this check in some weird edge-cases, why should you make that the default behavior?

Please just expose all PLC.ANYTHINGHERE directly as a Plutarch binding with no additional opinionated logic.

@L-as
Copy link
Member

L-as commented Oct 30, 2024

@colll78 you can still use punsafeBuiltin directly I think, but obviously this code is wrong. Missed in review I suppose.

@L-as
Copy link
Member

L-as commented Oct 30, 2024

You could always make a PR.

@colll78
Copy link
Author

colll78 commented Oct 31, 2024

You could always make a PR.

Lol I did make a PR, I was the one who introduced these functions to the Plutarch codebase in the first place (without any opinionated logic). Then my PR was reworked to add type safety and suddenly when I upgrade to the newer version my ex-unit benchmarks went up a ton.

@L-as
Copy link
Member

L-as commented Nov 1, 2024

...lol. I really think the core Plutarch implementation should be entirely separate from it's pseudo-standard-library anyway. It should only provide the most core things and everything else should be in plutarch-extra or similar.

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

No branches or pull requests

2 participants