Namespaced keysets #32
Conversation
0xd34df00d
commented
Nov 30, 2023
•
0xd34df00d
commented
- To see the specific tasks where the Asana app for GitHub is being used, see below:
- https://app.asana.com/0/0/1206050196253653
This is mostly needed because enforcing guards doesn't require the `b` builtin, and some contexts where enforcing's used actually don't have any `b` value around.
| @@ -102,7 +102,7 @@ evalModuleGovernance interp tl = do | |||
| lookupModule (Lisp._mInfo m) pdb mname >>= \case | |||
| Just targetModule -> do | |||
| term <- case _mGovernance targetModule of | |||
| KeyGov (KeySetName ksn) -> do | |||
| KeyGov (KeySetName ksn _mNs) -> do | |||
There was a problem hiding this comment.
I guess the namespace should be ignored here, but worth double-checking.
pact-core/Pact/Core/Syntax/Parser.y
Outdated
| @@ -141,7 +141,8 @@ ReplSpecial :: { SpanInfo -> ReplSpecialForm SpanInfo } | |||
| | load STR { ReplLoad (getStr $2) False } | |||
|
|
|||
| Governance :: { Governance ParsedName } | |||
| : StringRaw { KeyGov (KeySetName $1)} | |||
| : StringRaw { KeyGov (KeySetName $1 Nothing) } | |||
| | StringRaw '.' StringRaw { KeyGov (KeySetName (getStr $2) (Just (NamespaceName $1))) } | |||
There was a problem hiding this comment.
@jmcardon does this production make sense, and should un-namespaced keysets (the first production, that is) allowed in this context?
There was a problem hiding this comment.
I feel like we also need to check all of these changes against pact-corpus. I'm not sure how we executed the test in the past. Maybe @jmcardon can comment here?
| cond <- enforceKeyset oldKs | ||
| if cond then writeKs | ||
| else returnCEK cont handler (VError "enforce keyset failure" info) | ||
| Nothing | laxNs -> writeKs |
There was a problem hiding this comment.
A comment here would be beneficial
There was a problem hiding this comment.
Did that, but on a second reading I'd rather rename the binding from laxNs (that's indeed not the best name) to, say, ignoreNamespaces, as IMO comments about behavior of such implementation details (like, what the branch is doing) is typically a sign the names could be better chosen. How do you reckon doing that?
There was a problem hiding this comment.
yeah, I'm in favor of ignoreNamespaces 👍
pact-core/Pact/Core/Syntax/Parser.y
Outdated
| @@ -141,7 +141,8 @@ ReplSpecial :: { SpanInfo -> ReplSpecialForm SpanInfo } | |||
| | load STR { ReplLoad (getStr $2) False } | |||
|
|
|||
| Governance :: { Governance ParsedName } | |||
| : StringRaw { KeyGov (KeySetName $1)} | |||
| : StringRaw { KeyGov (KeySetName $1 Nothing) } | |||
| | StringRaw '.' StringRaw { KeyGov (KeySetName (getStr $2) (Just (NamespaceName $1))) } | |||
There was a problem hiding this comment.
I feel like we also need to check all of these changes against pact-corpus. I'm not sure how we executed the test in the past. Maybe @jmcardon can comment here?
Co-authored-by: rsoeldner <[email protected]>
