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: typos #1779

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

docs: typos #1779

wants to merge 5 commits into from

Conversation

futreall
Copy link

@futreall futreall commented Nov 9, 2024

Bug fixes and new features should include tests.

Typos / punctuation / trivial PRs are generally not accepted.

This pull request addresses a few documentation and code comments improvements to enhance clarity and correct minor inaccuracies. Specifically, it corrects typographical and grammatical errors in CHANGELOG.md, common-issues.md, and rkm0959.md. These changes do not alter functionality but aim to improve the readability and professionalism of the documentation.

Solution

The solution involved reviewing and updating the identified files to correct spelling, grammar, and punctuation errors. Each file was carefully edited to ensure that the corrections maintained the original intent and meaning of the content. Additionally, I added missing acknowledgments and updated the changelog for clarity.

PR Checklist

  • Added Tests (if applicable)
  • Added Documentation (as needed)
  • Breaking changes (no breaking changes introduced)

@nhtyy nhtyy added the documentation Improvements or additions to documentation label Nov 20, 2024
@nhtyy nhtyy changed the title fix typos in documentation files docs: typos Nov 20, 2024
Copy link
Contributor

@matthiasgoergens matthiasgoergens left a comment

Choose a reason for hiding this comment

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

Thanks for the effort! Please see comments.

(I'm not affiliated with succinct or SP1. I'm just an interested bystander.)

@@ -250,7 +250,7 @@ Then, as we move "down" the recursion tree, we assert that

This means that our `vk_root` check "propagates downwards" the tree.

## 5. [Medium] `cumulative_sum` needs to be observed when we observe the `permutation_commit`
## 5. [Medium] `cumulative sum` needs to be observed when we observe the `permutation_commit`
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove the underscore here? I think it's part of a variable name, and should be quoted as is.

@@ -18,7 +18,7 @@ fn duplexing(&mut self, builder: &mut Builder<C>) {
}
```

We are using the full sponge state for the output buffer. This is not an usual behavior for sponge constructions - only the rate part of the sponge state is used for the output usually. One possibility this opens up is that for valid hash result after a single permutation, it is easy to get the hash preimage. Note that this doesn't mean that it's easy to get the hash preimage for any desired hash output result, leading to the low severity.
We are using the full sponge state for the output buffer. This is not a usual behavior for sponge constructions - only the rate part of the sponge state is used for the output usually. One possibility this opens up is that for valid hash result after a single permutation, it is easy to get the hash preimage. Note that this doesn't mean that it's easy to get the hash preimage for any desired hash output result, leading to the low severity.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would even go with 'the' instead of 'a', or just drop the article completely.

Copy link
Author

Choose a reason for hiding this comment

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

@matthiasgoergens Thank you for pointing this out. Removing the underscore was a mistake. I'll revert it to the original format as it is indeed part of the variable name.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not quite sure it makes sense to change the audit report. It's a particular document about a particular moment in time, not a living document like the source code.

Copy link
Author

Choose a reason for hiding this comment

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

Got it, thanks for the feedback. I just wanted to fix a typo, but if it's inappropriate, I can revert the changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just a curious bystander, I don't know what SP1 folks consider as appropriate, sorry.

@@ -48,4 +48,4 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- readme cleanup ([#196](https://github.com/succinctlabs/sp1/pull/196))
- rename succinct to curta ([#192](https://github.com/succinctlabs/sp1/pull/192))
- better curta graphic ([#184](https://github.com/succinctlabs/sp1/pull/184))
- Initial commits
Copy link
Contributor

Choose a reason for hiding this comment

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

Your edit changes the meaning here. I am not sure the new meaning is what the original author intended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants