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

contract-sdk: Expose signature verification to wasm contracts #793

Merged
merged 1 commit into from
Apr 1, 2022

Conversation

jberci
Copy link
Contributor

@jberci jberci commented Feb 22, 2022

Closes #368

@codecov
Copy link

codecov bot commented Feb 22, 2022

Codecov Report

Merging #793 (8eea938) into main (64a33cc) will decrease coverage by 1.13%.
The diff coverage is 2.46%.

@@            Coverage Diff             @@
##             main     #793      +/-   ##
==========================================
- Coverage   73.14%   72.00%   -1.14%     
==========================================
  Files         114      116       +2     
  Lines        8846     8987     +141     
==========================================
+ Hits         6470     6471       +1     
- Misses       2353     2493     +140     
  Partials       23       23              
Impacted Files Coverage Δ
client-sdk/go/crypto/signature/secp256k1/signer.go 59.37% <0.00%> (-16.63%) ⬇️
client-sdk/go/modules/contracts/types.go 40.00% <ø> (ø)
contract-sdk/src/abi/env.rs 0.00% <0.00%> (ø)
contract-sdk/src/testing.rs 12.90% <0.00%> (-5.28%) ⬇️
contract-sdk/types/src/crypto.rs 0.00% <0.00%> (ø)
contract-sdk/types/src/lib.rs 100.00% <ø> (ø)
runtime-sdk/modules/contracts/src/abi/oasis/env.rs 70.58% <ø> (+16.42%) ⬆️
runtime-sdk/modules/contracts/src/abi/oasis/mod.rs 62.96% <ø> (ø)
runtime-sdk/modules/contracts/src/lib.rs 69.19% <ø> (ø)
runtime-sdk/src/crypto/signature/ed25519.rs 43.75% <0.00%> (-12.25%) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64a33cc...8eea938. Read the comment docs.

contract-sdk/src/env.rs Outdated Show resolved Hide resolved
@Yawning
Copy link
Contributor

Yawning commented Feb 24, 2022

Per discussion: The batch verification stuff should just go away, because it's a giant pain.

While I'm commenting on this:

The Ed25519 signature verification in oasis-core/runtime/src/common/crypto exposes our flavor of Ed25519 (with our janky domain separation), and our idea how to handle certain edge cases.

Is it ok that we are forcing this on end user code? I suppose that the circumstances that lead us to use something non-standard at the consensus side in the first place haven't really changed ("Ledger doesn't support Ed25519ctx/Ed25519ph"), but actual contract authors might want Ed25519pure (as opposed to Ed25519-prehashed-oasis-flavor-because-ledger).

If we do want this, I would recommend using one of ZIP-215/FIPS 186-5/Oasis flavored Ed25519pure, which would require additional code (or pulling in a crate), with a slight preference towards one of the latter two.

@kostko
Copy link
Member

kostko commented Feb 24, 2022

The Ed25519 signature verification in oasis-core/runtime/src/common/crypto exposes our flavor of Ed25519 (with our janky domain separation), and our idea how to handle certain edge cases.

Yeah we should definitely not impose our domain separation scheme and we should expose Ed25519pure here, possibly the ZIP-215 one?

@Yawning
Copy link
Contributor

Yawning commented Feb 28, 2022

Yeah we should definitely not impose our domain separation scheme and we should expose Ed25519pure here, possibly the ZIP-215 one?

I wish ZIP-215 was closer to the FIPS draft, though my preferred Ed25519 definition involves rejecting small-order-A.

@Yawning
Copy link
Contributor

Yawning commented Mar 1, 2022

Since we're going to expose Ed25519pure, the only primitive where a domain separation context makes sense is sr25519. Should we nix the argument for now, or leave it?

@kostko
Copy link
Member

kostko commented Mar 1, 2022

Since we're going to expose Ed25519pure, the only primitive where a domain separation context makes sense is sr25519. Should we nix the argument for now, or leave it?

Dropping it would make the interface more consistent, but I guess it could be useful for sr25519?

@jberci jberci force-pushed the jberci/feature/crypto branch 3 times, most recently from a99ebc5 to d25eff3 Compare March 11, 2022 14:25
@Yawning
Copy link
Contributor

Yawning commented Mar 16, 2022

The new API looks ok to me.

Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

This should also add parameters for controlling maximum sizes of inputs and set them to some sensible defaults.

contract-sdk/src/abi/env.rs Outdated Show resolved Hide resolved
contract-sdk/src/testing.rs Outdated Show resolved Hide resolved
runtime-sdk/modules/contracts/src/lib.rs Outdated Show resolved Hide resolved
runtime-sdk/src/crypto/signature/ed25519.rs Outdated Show resolved Hide resolved
runtime-sdk/src/crypto/signature/secp256k1.rs Outdated Show resolved Hide resolved
@jberci jberci force-pushed the jberci/feature/crypto branch 2 times, most recently from 87a1aed to 3d4597b Compare March 28, 2022 15:40
@jberci jberci marked this pull request as ready for review April 1, 2022 13:29
@jberci jberci requested review from pro-wh and ptrus as code owners April 1, 2022 13:29
@jberci jberci merged commit fc7029f into main Apr 1, 2022
@jberci jberci deleted the jberci/feature/crypto branch April 1, 2022 13:57
tjanez pushed a commit to oasisprotocol/cli that referenced this pull request Sep 20, 2022
tjanez pushed a commit to oasisprotocol/cli that referenced this pull request Oct 11, 2022
tjanez pushed a commit to oasisprotocol/cli that referenced this pull request Oct 11, 2022
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.

Native cryptography function imports
3 participants