-
Notifications
You must be signed in to change notification settings - Fork 20
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
runtime-sdk: Add sr25519 support in EVM precompiles #2073
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for oasisprotocol-oasis-sdk canceled.
|
087bccb
to
f8d82ba
Compare
@@ -60,6 +61,27 @@ impl PublicKey { | |||
.map_err(|_| Error::VerificationFailed) | |||
} | |||
|
|||
/// Verify a signature. | |||
pub fn verify_raw( |
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.
This is not in line with other signers, where you have /// Verify signature without using any domain separation scheme.
, so optionally we could think about renaming this method.
|
||
fn new_from_seed(seed: &[u8]) -> Result<Self, Error> { | ||
let seed = sha2::Sha512_256::digest(seed); | ||
let mut rng = rand_chacha::ChaChaRng::from_seed(seed.into()); |
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.
Where is this defined or taken from?
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.
Just an ad-hoc deterministic way of generating a key. There is no spec for generating something from a seed AFAIK and we need this in tests. But we could also move some of this to MemorySigner::new_test
and just forward to from_bytes
here?
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.
If this is only needed for tests, can we restrict it usage to test
only and try to avoid adding (chacha) dependencies.
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.
Actually, we need this (or similar) as otherwise it is harder to quickly generate valid keys. @CedarMist any thoughts on this?
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.
Once we define this function, we cannot change it as if someone started using this Signer
, new generation won't be compatible. So the best way would be to return signer in tests and error otherwise.
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.
used by the "generate" precompile, not just in tests. So it cannot error.
This should be mentioned in a comment as it is unusual that external code tightens implementation of the Signer
. Normally, the external code should adapt.
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.
The thing is that new_from_seed
should accept some sort of (random) seed without structure. But if we make it forward to from_bytes
here, it will require a specific structure.
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.
With the generate
precompile, I look at it as a Multiply base point by scalar
precompile.
Any modification to the scalar before point multiplication will cause problems.
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.
Can you provide this in terms of schnorrkel
?
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.
E.g. this could use MiniSecretKey
and expansion, similar to Ed25519? Then the question is which kind of expansion, uniform or ed25519-like?
f8d82ba
to
1a596b9
Compare
Constructing STROBE/Merlin transcript requires access to the underlying KeccakF[1600] permutation function, I'm wary that implementing sr25519 signing alone may not be enough if people end up reverting to Solidity to construct the message to be signed, or if the sr25519 signer makes assumption about the transcript which makes it impossible to use for other things. |
We can always introduce additional signature modes with different transcripts if needed. This is the easiest way to add into the existing framework, it would be good to know if it is sufficient for the use cases at hand. |
Fixes #1968