-
Notifications
You must be signed in to change notification settings - Fork 21
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 precompiles for key generation and signing #1176
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1176 +/- ##
==========================================
+ Coverage 64.65% 66.01% +1.35%
==========================================
Files 135 136 +1
Lines 12384 12960 +576
==========================================
+ Hits 8007 8555 +548
- Misses 4345 4373 +28
Partials 32 32
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
ebf7060
to
50a301d
Compare
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.
Thanks for this PR. I think the functionality overall looks solid, but it seems quite like there's some abstraction that describes all three variants in a way that ties them all together and reduces the need for tests.
One approach is to just have the one enum and its impl
but have big ol' match
arms that handle each case. This keeps things easy to understand and high performance with a slight decrease in readability.
Another approach create a trait called trait MemorySigner
(or something) containing provided implementations of all of these methods with associated types type Signer: Signer
and type Digest: Digest
(so like let mut digest = Self::Digest::new()
). You could then pass around a Box<dyn MemorySigner>
for polymorphism. This comes with cleaner separation of implementations, but less efficiency (not that it really matters when you're only calling the thing once).
A third approach that mixes the two above and is quite like what you're already doing here: enum dispatch for performance but a trait to keep the APIs in sync. There's even a macro for it (enum_dispatch)! I'm not sure if this one works with associated types, though.
I'd suggest:
- take a look at the different kinds of digests and signers you'll need (e.g.,
k256
+sha3
,ed25519_dalek
+sha2
) and see if they all implement a common trait (e.g., digest::Digest). If they do, you can simplify the implementation using associated types. - Try to collect everything into a trait and use
Box<dyn Signer>
for polymorphism to keep things simple. If you find that doesn't work, then you can bring back your enum but also bind the implementations together using a trait. - Look at
enum_dispatch
for your own edification. A tiny performance benefit isn't worth the supply chain risk of an extra dependency.
50a301d
to
5e32ee7
Compare
5e32ee7
to
7a9b60b
Compare
47fb41d
to
bdfffb0
Compare
/// Length of an EVM word, in bytes. | ||
pub const WORD: usize = 32; | ||
|
||
pub fn round_up(what: usize) -> usize { |
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.
pub fn round_up(what: usize) -> usize { | |
pub fn round_up_u5(what: usize) -> usize { |
is this a correct interpretation?
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.
Depends on what you mean by the u5. The function should round the number up to the nearest multiple of 32, so the bottom 5 bits are cleared.
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 looks overall good. Just one main question, though: I see that the precompile is still about signing messages rather than signing digests using separate digest functions. Am I mistaken in thinking this? @kostko
d66242c
to
95b5c2d
Compare
95b5c2d
to
6fdafee
Compare
6fdafee
to
efbc21d
Compare
7b1e85e
to
be27674
Compare
const WORD: usize = 32; | ||
|
||
/// The cost of a key pair generation operation, per method. | ||
static KEYPAIR_GENERATE_BASE_COST: Lazy<HashMap<SignatureType, u64>> = Lazy::new(|| { |
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.
please inline this as an if ... else if ... else
(or match)
}); | ||
|
||
/// The costs of a message signing operation. | ||
static SIGN_MESSAGE_COST: Lazy<HashMap<SignatureType, (u64, u64)>> = Lazy::new(|| { |
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.
please inline this as an if ... else if ... else
(or match)
impl SignatureType { | ||
pub fn as_int(&self) -> u8 { | ||
match self { | ||
Self::Ed25519_Oasis => 0, |
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 suggest that 0 be nothing for the same reason protobuf makes the zeroth item Unknown
(especially good for solidity where you can't test for presence).
} | ||
|
||
impl super::Signer for MemorySigner { | ||
fn new_from_seed(seed: &[u8]) -> Result<Self, Error> { |
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 isn't really so much of generate
as derivePublicKey
. Maybe this should invoke random_bytes
with seed
as the pers str?
guess I was too late |
No description provided.