-
Notifications
You must be signed in to change notification settings - Fork 243
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
feat: alt_bn128 curve #1028
feat: alt_bn128 curve #1028
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add tests to these. Also, if there are good examples of how to use these functions and references to the relevant specifications / descriptions about what these "alt" are, please, add links in the doc comment as well
@frol I wasn't sure if we should test the implementation here when it lives in nearcore, but I see it's done for other cyphers. Fair enough. I'll try to figure out/look up test vectors and explanations. Happy for hints on where to look since I'm not very knowledgeable about cryptography. |
The closest I got to finding test vectors is the test cases section here: https://eips.ethereum.org/EIPS/eip-196 Sadly, most of this is way over my head. |
@uint Fair point. Well, I am not familiar with this topic either, so I looked up nearcore tests: https://github.com/near/nearcore/blob/f5e8746ed85202bea244d3a2d4c698f7739d05bb/runtime/near-test-contracts/estimator-contract/src/lib.rs#L557-L720. Let's try porting those tests, as I am not comfortable shipping new functions without ever running them. |
@frol I think (?) what you linked to are gas benchmarks. They don't include expected outputs, but maybe I could get them by performing the same operations using an alternative implementation of this cypher. Worth a shot. At least we'll have the happy path covered. I still think we should include these correctness tests in nearcore only, since that's where the implementation lives. Since we only have bindings here, I could maybe add some very simple tests asserting that the bindings are pointing to existing host fns. What do you think? |
Yes, that is exactly what I had in mind. Just test the happy path.
Totally agree, but I want to catch the situation if nearcore breaks the behavior early.
I feel that calling the functions would be exactly the test (I don't suggest we copy all the test vectors from nearcore - call it once to test the interface is enough). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found more tests in nearcore: https://github.com/near/nearcore/blob/8cd095ffc98a6507ed2d2a8982a6a3e42ebc1b62/runtime/near-vm-runner/src/logic/tests/alt_bn128.rs#L87-L271
Added return value validation in the tests
Picks up where #188 left off to expose alt_bn128 functions to smart contract devs.
The implementation is already there in nearcore: near/nearcore#3971
I tested briefly that these are callable from test smart contracts, but if something's missing, let me know!