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

Namespaced Keysets #1011

Merged
merged 37 commits into from
Aug 9, 2022
Merged

Namespaced Keysets #1011

merged 37 commits into from
Aug 9, 2022

Conversation

emilypi
Copy link
Member

@emilypi emilypi commented Jun 27, 2022

See: #351

@michaeldoron59

This comment was marked as off-topic.

@emilypi
Copy link
Member Author

emilypi commented Jul 4, 2022

@michaeldoron59 please comment on the issue and not the PR.

src/Pact/Native/Capabilities.hs Outdated Show resolved Hide resolved
src/Pact/Native/Internal.hs Outdated Show resolved Hide resolved
src/Pact/Types/KeySet.hs Outdated Show resolved Hide resolved
src/Pact/Types/KeySet.hs Outdated Show resolved Hide resolved
src/Pact/Types/KeySet.hs Show resolved Hide resolved
src/Pact/Types/KeySet.hs Outdated Show resolved Hide resolved
@emilypi emilypi marked this pull request as ready for review August 2, 2022 20:33
@emilypi emilypi changed the title WIP: namespaced keysets Namespaced Keysets Aug 2, 2022
-- enforce the user guard and make sure we have privs
enforceGuard i ug
pure k
_ -> evalError' fi "Mismatching keyset namespaces")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error here could be a bit better. If KeySetName does not parse a Namespace, we're just going to fail because it's not namespaced.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure i could improve this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A more meaningful error message would be helpful, especially as most documents use the DisablePact44 behavior. Additionally, shouldn't the current namespace, if set, be enough? I.e.,

(namespace 'free)
(define-keyset "robert" ....

define the keyset implicitly as "free.robert"?

src/Pact/Types/ExpParser.hs Show resolved Hide resolved
src/Pact/Types/KeySet.hs Show resolved Hide resolved
} deriving (Eq, Ord, Show, Generic)

instance IsString KeySetName where
fromString ksn = case parseAnyKeysetName (T.pack ksn) of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this instance only for tests? This is such a strange instance.

really need to check all usages of fromString with this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can actually do without this and I"d be happy. IsString at least fails if it doesn't pass the parser test.

src/Pact/Eval.hs Show resolved Hide resolved
@@ -160,10 +160,11 @@ principalParser (getInfo -> i) = kParser
guardToPrincipal :: Guard (Term Name) -> Eval e Principal
guardToPrincipal = \case
GPact (PactGuard pid n) -> pure $ P pid n
-- TODO later: revisit structure of principal k and w accounts in light of namespaces
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to address this: Do we change the principal of the keyset ref guard?

@emilypi emilypi merged commit 98dc6db into master Aug 9, 2022
@emilypi emilypi deleted the feat/namespaced-keysets branch August 9, 2022 18:58
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

Successfully merging this pull request may close these issues.

None yet

4 participants