Skip to content

Conversation

ianbjacobs
Copy link
Collaborator

@ianbjacobs ianbjacobs commented Oct 7, 2021

Editorial. Addresses #140.


Preview | Diff

Copy link
Member

@samuelweiler samuelweiler left a comment

Choose a reason for hiding this comment

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

I still think reordering the section will help the reader

@ianbjacobs
Copy link
Collaborator Author

@samuelweiler, thanks for the review and additional comment. I hope this update does the trick.

@samuelweiler
Copy link
Member

this resolves the suggestion.

One additional suggestion: especially since "cryptogram" isn't really defined in the spec, can you provide a citation back to the verification algorithm - show where this check is performed? (8.1, #2, #5? Also, should the sub-steps have letters instead of numbers?) And maybe a different term is still more appropriate?

I also think there are more opportunities for trimming some words, though it's not necessary.

@ianbjacobs
Copy link
Collaborator Author

@samuelweiler,

You mentioned a citation back to the verification algorithm. The proposed text includes a link to "8.1" but uses different verbiage to refer to that section. (Also, I did not not find other references to "verifying the cryptogram", so this seems to be the only one.)

I hope this address your request for a citation.

@samuelweiler
Copy link
Member

Looks good to me.

@samuelweiler samuelweiler merged commit cd8781c into main Nov 5, 2021
github-actions bot added a commit that referenced this pull request Nov 5, 2021
SHA: cd8781c
Reason: push, by @samuelweiler

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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