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

docs: remove TODO comments for attestation type and path #122

Merged
merged 1 commit into from
Feb 21, 2019

Conversation

bdewater
Copy link
Collaborator

The W3C spec states at the end of every verification procedure for a attestation type:

If successful, return implementation-specific values representing attestation type and attestation trust path

Thus the TODO comment makes no sense, we need to call valid? to make this data available

The W3C spec states at the end of every verification procedure for a attestation type:

> If successful, return implementation-specific values representing attestation type <x> and attestation trust path <y>

Thus the TODO comment makes no sense, we need to call `valid?` to make this data available

[ci skip]
@grzuy
Copy link
Contributor

grzuy commented Feb 21, 2019

Hi @bdewater,

My reasoning behind those TODO comments was something like:

Wouldn't be better for the gem to "just call verification and then return whatever the result was" instead of "returning nil (like saying I don't know)", in case #attestation_type is called before (or even without) calling valid? or verify?

We'll still match the spec. Just easing the ruby gem usage experience.

@bdewater
Copy link
Collaborator Author

Ah, I see what you mean now. However I'm a little wary of this 'friendly' behaviour when it comes to failure cases. Would we return nil for attestation type in that case? If so, do we actually provide a tangible benefit for an app developer using this gem if the result can still be that?

IMO it's not an unreasonable ask to a developer to use the APIs as intended/correctly :) and right now I'm leaning towards making that more explicit by raising in case you call things in the wrong order, but I'm open to other ideas.

@grzuy
Copy link
Contributor

grzuy commented Feb 21, 2019

Let's remove the TODO by merging this and continue discussion in an issue :-)

@grzuy grzuy merged commit b0ee2cb into cedarcode:master Feb 21, 2019
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

2 participants