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

Has capability test #58

Merged
merged 15 commits into from
Apr 4, 2022
Merged

Has capability test #58

merged 15 commits into from
Apr 4, 2022

Conversation

nichoth
Copy link
Contributor

@nichoth nichoth commented Mar 21, 2022

This PR adds some tests for the hasCapability method. These were things that I was curious about when I was looking at how to validate permissions via UCAN. There is still one question I had, about the originator field. I made a test that currently fails -- https://github.com/nichoth/ts-ucan/blob/f2fde61b72adcd0a434e5ce1ba91cfe1d834fba1/tests/attenuation.test.ts#L296 -- the question is how is originator used? It doesn't seem to affect the return value of hasCapability. I'm not sure if this is not implemented yet or if my test is wrong in some way.

Thanks. Hopefully this is helpful and not creating needless work 😬

@matheus23
Copy link
Member

Thanks, that's an amazing contribution.

the question is how is originator used? It doesn't seem to affect the return value of hasCapability. I'm not sure if this is not implemented yet or if my test is wrong in some way.

Yes, this is a bug. Whoops! hasCapability should check the originator to make sure it matches what's passed in and also check that the time bounds work out. All of that instead of running delegateCapabilityInfo... My bad.

Your contribution is amazing. You've created a failing test case. We'll use that to implement a fix for this.

@icidasset
Copy link
Member

@nichoth Any chance you can rebase this on main? It should only be two merge conflicts, because the path of ./emailCapabilities has changed to ./capabilities/email, and I changed emailCaps = Array.from( to be multiline.

Would love to get this in so we can fix hasCapability 🙏

@nichoth
Copy link
Contributor Author

nichoth commented Apr 1, 2022

Ok I think it has been rebased & merged properly. My brain started overheating thinking about the branches, PRs, etc, but hopefully I did that correctly

Copy link
Member

@icidasset icidasset left a comment

Choose a reason for hiding this comment

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

Thanks! 🙌 I'll merge this next Monday and will make sure all the tests pass.

@nichoth
Copy link
Contributor Author

nichoth commented Apr 1, 2022

I just fixed the type checking with the updates, however there is one type I wasn't sure about -- https://github.com/nichoth/ts-ucan/blob/hasCapability-test/tests/attenuation.test.ts#L234

It is telling me

Property 'email' does not exist on type 'Capability'.ts(2339)

image

@nichoth
Copy link
Contributor Author

nichoth commented Apr 1, 2022

Update -- I have fixed the above error. It was a mixup with the expected object shapes for the capabilities.

@nichoth nichoth force-pushed the hasCapability-test branch 2 times, most recently from 966b1b2 to 61928fc Compare April 1, 2022 17:51
@matheus23
Copy link
Member

@nichoth this PR is almost good to go. I proposed some changes to this PR in another PR (see the mention above this comment 👆 ).

Once we've merged that PR, we can merge this PR 🎉

Copy link
Member

@matheus23 matheus23 left a comment

Choose a reason for hiding this comment

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

Okay. I'd love to merge this.

With my changes this also fixes #60

I'd love to see another quick look at this @icidasset 🙏

@icidasset
Copy link
Member

Looks good! 👏

@icidasset icidasset merged commit 1a7d814 into ucan-wg:main Apr 4, 2022
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.

3 participants