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

[Ed25519] Wrap the return value of Ed25519.sign and .verify with Right #4992

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

Conversation

jaredly
Copy link
Contributor

@jaredly jaredly commented May 21, 2024

Summary:

I haven't been able to figure out how to hook my new primops up to the nativeEvalInContext pipeline, but it looks like they're all set for compile.native. I just needed to wrap the return value in Right, and the following tests now pass:

zseed = 0xs0000000000000000000000000000000000000000000000000000000000000000
zpublic = 0xs3b6a27bcceb6a42d62a3a8d02a6f0d73653215771de243a63ac048a18b59da29
zsig = 0xs8f895b3cafe2c9506039d0e2a66382568004674fe8d237785092e40d6aaf483e4fc60168705f31f101596138ce21aa357c0d32a064f423dc3ee4aa3abf53f803

ed25519Tests = main do
    checkEqual "Ed25519 Sign" (crypto.Ed25519.sign.impl zseed zpublic 0xs) (Right 0xs8f895b3cafe2c9506039d0e2a66382568004674fe8d237785092e40d6aaf483e4fc60168705f31f101596138ce21aa357c0d32a064f423dc3ee4aa3abf53f803)
    checkEqual "Ed25519 Verify" (crypto.Ed25519.verify.impl zpublic 0xs zsig) (Right true)

Issue: XXX-XXXX

Test plan:

Add the above tests to the workspace, and then do compile.native ed25519Tests ed-test.
The file ~/.cache/unisonlanguage/racket-tmp/ed-test.rkt will then be produced, and running:
racket ~/.cache/unisonlanguage/racket-tmp/ed-test.rkt produces a successful test run!

@jaredly jaredly self-assigned this May 21, 2024
@jaredly jaredly marked this pull request as ready for review May 21, 2024 11:32
@jaredly jaredly requested a review from dolio May 21, 2024 11:32
@dolio
Copy link
Contributor

dolio commented May 21, 2024

I'm a bit confused. These aren't in the list of wrappers, right? In which case I think you should just forget about defining the FOp version and do the builtin directly (which I eventually want to do with most things).

But also, it works with compile.native and not with run.native? That'd be rather surprising.

@jaredly
Copy link
Contributor Author

jaredly commented May 23, 2024

@dolio
image

this is what I get from run.native, whereas compile.native produces a racket file that works just fine.

@dolio
Copy link
Contributor

dolio commented May 23, 2024

I can only think of two ways that'd happen.

  1. The unison-runtime program is using a stale library, and needs to be rebuilt.
  2. Whatever contains the builtin isn't required by the module generated during dynamic code loading.

The second shouldn't happen if the builtin is in one of the standard places, though (primops or simple-wrappers for example).

@dolio
Copy link
Contributor

dolio commented May 23, 2024

BTW, do you have some local work that isn't pushed? It doesn't seem to me like what's in the PR should actually work. If the FOp returns a unison Either, I'd expect a wrapper to bomb on it. And if there isn't a wrapper, and only the FOp is defined, I'd expect to just get the unbound identifier error all the time.

@jaredly
Copy link
Contributor Author

jaredly commented May 23, 2024

oh so I had manually edited some generated code to make a wrapper that didn't do anything. I'll be removing the FOp like you suggested.

@jaredly
Copy link
Contributor Author

jaredly commented May 24, 2024

ok @dolio this is working now! I just needed to regenerate the racket library.

@dolio
Copy link
Contributor

dolio commented May 24, 2024

These are just raising racket exceptions, right? Is that how they're supposed to work in the error cases? Seems weird that they return an Either if so.

@jaredly
Copy link
Contributor Author

jaredly commented May 24, 2024

The original unison impl has type Either, so I'm matching that. I'm going to be AFK starting monday until july 1st, so I probably won't be able to fix it up (to return unison errors) before I head out.

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