Principals #22

0xd34df00d · 2023-10-20T02:56:10Z

create-principal
is-principal
typeof-principal
validate-principal

0xd34df00d · 2023-10-27T05:19:11Z

pact-core/Pact/Core/Gas.hs

+coreGasPerLegacyGas :: Semiring r => r
+coreGasPerLegacyGas = fromNatural 1000


This is super ugly and I'd like this to go away, but we'll need to settle down with proper gas accounting first.

jmcardon

Good work so far!

In general, the main changes are:

Removing principals are pactvalues. They are not. I like the idea though!!! I think we need to pitch it to the team and discuss how we could work with it moving forward.
Remove all uses of gas. Gas is a task on its own. We will work on gas overall for pact-core as a large task.

jmcardon · 2023-10-27T05:13:21Z

pact-core/Pact/Core/Type.hs

@@ -111,6 +114,9 @@ pattern TyBool = TyPrim PrimBool
 pattern TyString :: Type
 pattern TyString = TyPrim PrimString

+pattern TyPrincipal :: Type
+pattern TyPrincipal = TyPrim PrimPrincipal


This needs to be removed until we settle on things actually returning principals as a type

jmcardon · 2023-10-27T05:13:31Z

pact-core/Pact/Core/Type.hs

@@ -53,6 +54,7 @@ data PrimType =
  PrimString |
  PrimGuard |
  PrimTime |
+  PrimPrincipal |


Ditto here, delete this

jmcardon · 2023-10-27T05:13:35Z

pact-core/Pact/Core/Type.hs

@@ -16,6 +16,7 @@ module Pact.Core.Type
 , pattern TyTime
 , pattern TyBool
 , pattern TyString
+ , pattern TyPrincipal


Principal "type" needs to be removed, at least for now.

jmcardon · 2023-10-27T05:14:14Z

pact-core/Pact/Core/IR/Eval/RawBuiltin.hs

+createPrincipalForGuard :: (MonadGas m) => Guard FullyQualifiedName PactValue -> m Pr.Principal
+createPrincipalForGuard g = do
+  -- TODO make gas charging actually go through the model
+  chargeGas coreGasPerLegacyGas


Remove all uses of gas. Gas in core is not implemented yet.

Moreover, this is my fault, I should clarify: we do not have to match legacy on gas.

jmcardon · 2023-10-27T05:15:31Z

pact-core/Pact/Core/IR/Eval/RawBuiltin.hs

+    mkHash bss = do
+      let bs = mconcat bss
+      -- the original pact impl charged 1 gas per 64 bytes of hashing,
+      void $ chargeGas $ coreGasPerLegacyGas <> Gas (fromIntegral (BS.length bs) * coreGasPerLegacyGas `quot` 64)


Here as well, remove the gas code.

jmcardon · 2023-10-27T05:16:40Z

pact-core/Pact/Core/IR/Eval/RawBuiltin.hs

+  chargeGas coreGasPerLegacyGas
+  case g of
+    GKeyset (KeySet ks pf) -> case (toList ks, predicateToString pf) of
+      ([k], "keys-all") -> pure $ Pr.K k


no need to predicate-to-string, you can match on the predicate itself.

jmcardon · 2023-10-27T05:18:33Z

pact-core/Pact/Core/StableEncoding.hs

@@ -184,6 +198,7 @@ instance J.Encode (StableEncoding PactValue) where
    PModRef mr -> J.build (StableEncoding mr)
    PCapToken _ct -> error "not implemented"
    PTime pt -> J.build (StableEncoding pt)
+    PPrincipal pr -> J.build (StableEncoding pr)


Remove, principals are not pact values yet. We don't produce principals are their own pactValues

jmcardon · 2023-10-27T05:19:34Z

pact-core/Pact/Core/Gas.hs

@@ -37,6 +38,9 @@ newtype Gas
  deriving (Semigroup, Monoid) via (Sum Word64)
  deriving (Semiring, Enum) via Word64

+coreGasPerLegacyGas :: Semiring r => r


No gas just yet. Remove.

jmcardon · 2023-10-27T05:19:44Z

pact-core/Pact/Core/Errors.hs

@@ -231,6 +232,7 @@ instance Pretty ArgTypeError where
    ATETable -> "[table]"
    ATEClosure -> "[closure]"
    ATEModRef -> "[modref]"
+    ATEPrincipal -> "[principal]"


Remove here as well.

0xd34df00d · 2023-10-31T03:35:02Z

@jmcardon addressed, thanks for the review! De-gassified and de-pactvalue-tized.

rsoeldner · 2023-10-31T18:31:14Z

pact-core/Pact/Core/Guards.hs

@@ -28,12 +32,18 @@ newtype PublicKeyText = PublicKeyText { _pubKey :: Text }
 instance Pretty PublicKeyText where
  pretty (PublicKeyText t) = pretty t

+renderPublicKeyText :: PublicKeyText -> Text


I feel like this is unnecessary and prefer the usage of _pubKey which we might rename as _publicKeyText ?

I wanted to make the intent of (stable-)rendering it explicit. And shall we ever change/extend the representation, that'd be easier to achieve.

rsoeldner · 2023-10-31T18:33:42Z

pact-core/Pact/Core/IR/Eval/RawBuiltin.hs

@@ -8,6 +8,7 @@
 {-# LANGUAGE DerivingVia #-}
 {-# LANGUAGE ConstraintKinds #-}
 {-# LANGUAGE MultiWayIf #-}
+{-# LANGUAGE ImportQualifiedPost #-}


Suggested change

{-# LANGUAGE ImportQualifiedPost #-}

Personal preference — I find this syntax more readable, especially where otherwise you'd have

import Data.Vector(Vector) import Data.Text(Text) -- bunch of other unqualified imports import qualified Data.Vector as V import qualified Data.Text as T -- bunch of other qualified imports

while with the post-syntax it's easier to see what gets imported from a given package/module when everything's grouped by the actual module name.

In fact, I did think of making a pass converting everything to ImportQualifiedPost (of course, after discussing this with the rest of the team when we'll have some time), so I can surely remove this for now for consistency.

rsoeldner · 2023-10-31T18:34:25Z

pact-core/Pact/Core/IR/Eval/RawBuiltin.hs

@@ -55,6 +58,7 @@ import Pact.Core.Persistence
 import Pact.Core.DefPacts.Types
 import Pact.Core.Environment
 import Pact.Core.Capabilities
+import Pact.Core.Principal qualified as Pr


I think throughout the codebase, we have not used it and I would personally prefer the standard way

Suggested change

import Pact.Core.Principal qualified as Pr

import qualified Pact.Core.Principal as Pr

also is this actually needed? Most of pact-core can be just imported without name clash

Yep, the C ctor clashes with Pact.Core.IR.Eval.Runtime.C, for example. I could've renamed the constructors themselves into PrC/etc, but I wanted to keep consistent with the old version, and personally I prefer disambiguating at module import time (oh, I wish haskell had agda's import renames!)

rsoeldner · 2023-10-31T18:37:37Z

pact-core/Pact/Core/IR/Eval/RawBuiltin.hs

+    GUserGuard (UserGuard f args) -> do
+      h <- mkHash $ map encodeStable args
+      pure $ Pr.U (Pretty.renderText f) (hashToText h)
+      -- TODO orig pact gets here ^^^^ a Name


Todo still pending?

Yep. I think we agreed to sort this out later on when dealing with Names more thoroughly.

rsoeldner · 2023-10-31T18:40:20Z

pact-core/Pact/Core/IR/Eval/RawBuiltin.hs

+      let args' = map encodeStable args
+          f' = T.encodeUtf8 $ renderQualName $ fqnToQualName f
+          pid' = T.encodeUtf8 . renderDefPactId <$> pid
+      h <- mkHash $ f' : args' ++ maybe [] pure pid'


Could be expressed using maybeToList?

Great suggestion, thanks!

rsoeldner · 2023-10-31T18:42:35Z

pact-core/Pact/Core/IR/Eval/RawBuiltin.hs

+      h <- mkHash $ f' : args' ++ maybe [] pure pid'
+      pure $ Pr.C $ hashToText h
+  where
+    mkHash bss = pure $ pactHash $ mconcat bss


why the Monad constraint and use of pure? I feel like here a type signature would be beneficial

That's for when we'll eventually have gas. It was all in MonadGas originally, but MonadGas was removed recently, so I changed that to Monad, while still keeping monadic syntax, so that (re)introducing gas would require less changes later on.

LMK if you'd nevertheless like to purify this function (for now?)

Could just make it pure for now.

rsoeldner · 2023-10-31T18:47:07Z

pact-core/Pact/Core/Principal.hs

@@ -0,0 +1,175 @@
+{-# LANGUAGE ImportQualifiedPost #-}


rsoeldner · 2023-10-31T18:50:59Z

pact-core/Pact/Core/Principal.hs

+nameMatcher = asMatcher $ qualifiedNameMatcher
+                      <|> bareNameMatcher
+  where
+    bareNameMatcher :: Parser ()


What is the reason, we use the Parsers package and not plain attoparsec here?

There are some things missing in atto. Some are easy to reimplement (like oneOf), others are a tad more involved (like ident). Since the legacy pact used parsers, depending on that package in pact-core as well both simplifies the implementation and helps ensure backwards compatible behavior.

Tbh, we don't need parsers either to parse an ident either but we can punt this to later.

jmcardon · 2023-11-13T16:21:59Z

pact-core/Pact/Core/IR/Eval/RawBuiltin.hs

+    GModuleGuard (ModuleGuard mn n) -> pure $ Pr.M mn n
+    GUserGuard (UserGuard f args) -> do
+      h <- mkHash $ map encodeStable args
+      pure $ Pr.U (Pretty.renderText f) (hashToText h)


Remove Pretty.renderText here and replace with renderQualName (fqnToQualName f)

jmcardon · 2023-11-13T16:24:41Z

pact-core/Pact/Core/IR/Eval/RawBuiltin.hs

+      h <- mkHash $ f' : args' ++ maybe [] pure pid'
+      pure $ Pr.C $ hashToText h
+  where
+    mkHash bss = pure $ pactHash $ mconcat bss


Could just make it pure for now.

jmcardon · 2023-11-13T16:25:36Z

pact-core/Pact/Core/Principal.hs

+nameMatcher = asMatcher $ qualifiedNameMatcher
+                      <|> bareNameMatcher
+  where
+    bareNameMatcher :: Parser ()


Tbh, we don't need parsers either to parse an ident either but we can punt this to later.

0xd34df00d · 2023-11-13T17:28:26Z

@jmcardon done! Except for the parsers — let's indeed do this later.

0xd34df00d added 30 commits October 16, 2023 14:53

Some initial principals scaffolding

04df6ae

Copy the Principal type over from pact

76e6d24

Principal needs to be a part of PactValue

8078981

Error kind for principals

6fe6e7a

Depend on pact-json

a899649

Expose gas units to simplify porting over legacy pact gassing

cf26cb4

Generalize KSPredicate conversion

e5c4fb8

instance J.Encode (StableEncoding Principal)

80d1e26

Some progress on coreCreatePrincipal

9f00c76

Be more explicit about QN/FQN rendering

b9c6d7c

Expose and use renderPactId for hashing

2c3073d

Expand on the comment re FQN/names (and no need to convert FQN to QN)

01b2f97

Implement mkPrincipalIdent

7f2f6d7

Add a TODO about gassing principals

f90b604

Forgot the module in .cabal

7ada5c3

A tad more concise StableEncoding for Principals

66d3650

Refactor out principal creation since it's also needed for validation

51fd8a8

pattern PBool

2ef65fa

Add validate-principal

592ea1b

Return a VString from create-principal for backcompat

d46b91e

Some progress on parsing principals

f7311ae

{qualified,bare}NameParser's needn't pollute the top-level namespace

11dff88

More precise export list

1a8e579

Parse r: principals

5ca5af7

Parse u: principals

75a1d08

Less copypaste

d2f6679

Almost all of the rest of the parsers are easy

3f85a0c

M principals parsing also done, and that concludes parsing

fa43e42

Update deps in pact-core.cabal

a25a93b

VPactValue . PBool ~ VBool

cc233c2

0xd34df00d added 2 commits October 26, 2023 13:13

is-principal done

4cb6dce

Showing principal types

919927a

0xd34df00d requested review from rsoeldner and jmcardon October 27, 2023 04:35

Add repl tests

4bd4d26

0xd34df00d marked this pull request as ready for review October 27, 2023 04:35

Parsing principals

37717f7

0xd34df00d commented Oct 27, 2023

View reviewed changes

jmcardon reviewed Oct 27, 2023

View reviewed changes

0xd34df00d added 4 commits October 30, 2023 15:02

Un-gas principals

cb5fc24

Remove principals from primitive types as per PR comments

b756a96

Delay predicateToString as much as possible

f02f0d0

Merge remote-tracking branch 'origin/master' into gr/principals

edfb40b

renderPactId → renderDefPactId for consistency

4630f2d

rsoeldner reviewed Oct 31, 2023

View reviewed changes

0xd34df00d added 2 commits October 31, 2023 14:34

Un-ImportPostQualified-everything for now

086dcea

Use maybeToList

f1b7893

jmcardon reviewed Nov 13, 2023

View reviewed changes

0xd34df00d added 2 commits November 13, 2023 11:22

Merge remote-tracking branch 'origin/master' into gr/principals

87767d6

Purify createPrincipalForGuard

df81115

0xd34df00d added 2 commits November 13, 2023 11:29

Avoid renderText

57ced6d

Enable a part of principals tests that work thanks to having ns now!

9155d43

0xd34df00d merged commit f274e5a into master Nov 14, 2023

0xd34df00d deleted the gr/principals branch November 14, 2023 02:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Principals #22

Principals #22

0xd34df00d commented Oct 20, 2023 •

edited

Loading

0xd34df00d Oct 27, 2023

jmcardon left a comment

jmcardon Oct 27, 2023

jmcardon Oct 27, 2023

jmcardon Oct 27, 2023

jmcardon Oct 27, 2023

jmcardon Oct 27, 2023

jmcardon Oct 27, 2023

jmcardon Oct 27, 2023

jmcardon Oct 27, 2023

jmcardon Oct 27, 2023

0xd34df00d commented Oct 31, 2023

rsoeldner Oct 31, 2023

0xd34df00d Oct 31, 2023

rsoeldner Oct 31, 2023

0xd34df00d Oct 31, 2023

rsoeldner Oct 31, 2023

0xd34df00d Oct 31, 2023

rsoeldner Oct 31, 2023

0xd34df00d Oct 31, 2023

rsoeldner Oct 31, 2023

0xd34df00d Oct 31, 2023

rsoeldner Oct 31, 2023

0xd34df00d Oct 31, 2023

jmcardon Nov 13, 2023

rsoeldner Oct 31, 2023

rsoeldner Oct 31, 2023

0xd34df00d Oct 31, 2023

jmcardon Nov 13, 2023

jmcardon Nov 13, 2023

jmcardon Nov 13, 2023

jmcardon Nov 13, 2023

0xd34df00d commented Nov 13, 2023

		coreGasPerLegacyGas :: Semiring r => r
		coreGasPerLegacyGas = fromNatural 1000

	import Pact.Core.Principal qualified as Pr
	import qualified Pact.Core.Principal as Pr

Principals #22

Principals #22

Conversation

0xd34df00d commented Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

jmcardon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xd34df00d commented Oct 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xd34df00d commented Nov 13, 2023

0xd34df00d commented Oct 20, 2023 •

edited

Loading