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

add replacement for tweetNaCL exercise #138

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

squell
Copy link
Member

@squell squell commented Dec 20, 2024

This is a backport from rust-training material.

@hdoordt
Copy link
Member

hdoordt commented Dec 20, 2024

Great work on the exercise! Looks really good.

I don't see why this needs to replace the TweetNaCl exercise, though. It's nice for people to be able to pick more exercises

@squell
Copy link
Member Author

squell commented Dec 23, 2024

Great work on the exercise! Looks really good.

I don't see why this needs to replace the TweetNaCl exercise, though. It's nice for people to be able to pick more exercises

I mean sure if you want, we can keep it, but the tweetnacl exercise has some didactic problems:

  • There's a extern declaration for a randombytes() in tweetnacl.c that's not provided. On Linux/MacOS that turns out to not be an issue if you don't reference the functions that call it, but on Windows we've seen people run into problems.

  • tweetNaCL is actually a very poor example of a C library. If people think, well that's nice, let's also implement crypto_hash_sha256_tweet you're off into the deep end (you notice there's not a literal definition of crypto_hash_shaXYZ_tweet. If you persevere you see that you can make it work by editing tweetnacl.h in exactly the right way---but then you will loose the implementation for crypto_hash_sha512_tweet. In tweetNaCL, a lot of macro trickery was involved to squeeze it in 100 tweets and this is the downside.

  • There's not really a tangible way for students to feel that the bindings are actually doing something useful since by definition a SHA512 will just be a collection of 64 meaningless bytes. With Qoi it's pretty rewarding: you get an image.

@hdoordt
Copy link
Member

hdoordt commented Jan 21, 2025

@squell I agree with you that this new exercise is an improvement. The reason that I'd like to keep the old one, is that I'd like for teach-rs to be a library of composable educational material. We can 'bless' certain parts by including them in our predefined tracks, but there's no harm in keeping 'unblessed' material around IMO.

If you can alter this PR such that the tweetnacl exercise is preserved, I'll gladly mash that 'merge' button

@squell
Copy link
Member Author

squell commented Jan 30, 2025

That sounds reasonable (you're an inclusionist I see, I'm probably more a deletionist?).

@squell squell changed the title replace tweetNaCL exercise with Qoi interface add replacement for tweetNaCL exercise Jan 30, 2025
@squell
Copy link
Member Author

squell commented Jan 30, 2025

I'm a little unsure if I put the exercise_ref's in the correct spot -- they also weren't there in the tweetnacl exercise to help guide me.

@hdoordt
Copy link
Member

hdoordt commented Jan 31, 2025

@squell Looks good at first glance. You can always try generating a track containing this exercise and see if it came out well

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.

2 participants