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

SSZ and Merkleization wiki pages #244

Merged
merged 41 commits into from
May 7, 2024
Merged

SSZ and Merkleization wiki pages #244

merged 41 commits into from
May 7, 2024

Conversation

thogiti
Copy link
Contributor

@thogiti thogiti commented May 2, 2024

Wiki PR Checklist

  • SSZ wiki page
  • Merkleization wiki page

@thogiti thogiti requested review from taxmeifyoucan and raxhvl May 2, 2024 00:47
@thogiti thogiti mentioned this pull request May 2, 2024
@raxhvl
Copy link
Member

raxhvl commented May 2, 2024

I'll review this after our call today.

docs/wiki/CL/merkleization.md Outdated Show resolved Hide resolved
docs/wiki/CL/merkleization.md Outdated Show resolved Hide resolved
docs/wiki/CL/merkleization.md Outdated Show resolved Hide resolved
docs/wiki/CL/merkleization.md Outdated Show resolved Hide resolved
@thogiti
Copy link
Contributor Author

thogiti commented May 6, 2024

@taxmeifyoucan This is ready to be merged. Thx.

@taxmeifyoucan
Copy link
Contributor

Thanks @thogiti! I lgtm, I was just waiting whether @raxhvl or @angaz add a review. Let's give them a bit more time, otherwise I'll merge

Copy link
Contributor

@angaz angaz left a comment

Choose a reason for hiding this comment

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

This is awesome! So much information. So much detail. It's next level.

I just have a few nitpicks.

I'm not sure if we have a policy on line length. I generally go with 80-120. A formatter can probably fix that if it's something we would like to have. Not the end of the world. Just nice for someone who doesn't have wrapping on their editor.

It looks like the colour scheme for the charts don't really work well with dark mode. I'm not sure what to do about that. Maybe just use outlines? Or specify the text colour as well so it isn't converted into a lighter colour in dark mode.
image

wordlist.txt Outdated Show resolved Hide resolved
docs/wiki/CL/SSZ.md Outdated Show resolved Hide resolved
@thogiti
Copy link
Contributor Author

thogiti commented May 7, 2024

@taxmeifyoucan @angaz I pushed a commit with all the above suggestions, removed custom colors on the mermaid graphs, standardized the lists formatting, etc.

This is ready to be merged.

@taxmeifyoucan
Copy link
Contributor

Thanks for all the reviews and updates guys!

@taxmeifyoucan taxmeifyoucan merged commit affeb93 into main May 7, 2024
1 of 2 checks passed
Copy link

github-actions bot commented May 7, 2024

Hi @thogiti,

Following typos were found in the pull request:

  • 📄 ./docs/wiki/Cryptography/post-quantum-cryptography.md:
    1. HSP
    2. KYBER
    3. Kyber
  • 📄 ./docs/wiki/research/eODS.md:
    1. liquidty
    2. collateralized
    3. unincentivised
  • 📄 ./docs/wiki/research/roadmap.md:
    1. bootstraping
    2. frontrunning

ℹ️ Here's how to fix them:

  • Fix typos: Open the relevant files and fix any identified typos.
  • Update wordlist: If a flagged word is actually a project-specific term add it to wordlist.txt in the project root.
    Each word should be listed on a separate line. Learn more.
  • 🚧 Remember:
    • When adding new words it MUST NOT have any spaces or special characters within or around it.
    • wordlist is NOT case sensitive.
    • Use backticks to quote code variables so as to not bloat the wordlist.

docs/wiki/CL/SSZ.md Show resolved Hide resolved
docs/wiki/CL/SSZ.md Show resolved Hide resolved
docs/wiki/CL/SSZ.md Show resolved Hide resolved
docs/wiki/CL/SSZ.md Outdated Show resolved Hide resolved
@raxhvl
Copy link
Member

raxhvl commented May 7, 2024

@thogiti I submited the review after the merge. Small nits. It's a great article overall. Thank you.

@taxmeifyoucan
Copy link
Contributor

Sorry for merging too soon! Please push more updates to cl-architecture branch and I will merge it

wordlist.txt Show resolved Hide resolved
wordlist.txt Show resolved Hide resolved
wordlist.txt Show resolved Hide resolved
@thogiti
Copy link
Contributor Author

thogiti commented May 7, 2024

@thogiti I submited the review after the merge. Small nits. It's a great article overall. Thank you.

Love it. Our wiki will be better with more reviews, feedback and comments. Thank you @raxhvl, @angaz, and @taxmeifyoucan for the feedback.

I will apply the changes/updates in a new PR and request you to merge it.

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.

4 participants