Use signing/recovery from fuel_crypto in fuels_signers #165

Br1ght0ne · 2022-03-21T13:55:35Z

Close #65.

Things left:

impl FromStr for fuel_crypto::Signature
port remaining tests that require Signature::from_str
fix tests failing after rebase

digorithm · 2022-03-21T14:43:21Z

Since there's still impl FromStr for fuel_crypto::Signature so I can port tests straight away left, shouldn't we keep this PR as a draft? Still, I'll be doing an initial review soon 😄

Br1ght0ne · 2022-03-21T15:02:42Z

Since there's still impl FromStr for fuel_crypto::Signature so I can port tests straight away left, shouldn't we keep this PR as a draft? Still, I'll be doing an initial review soon 😄

Whoops, pressed too early. Will add this impl soon :)

Br1ght0ne · 2022-03-29T15:57:45Z

Rebased on master to solve conflict in Cargo.lock.

fuels-signers/src/lib.rs

Br1ght0ne · 2022-03-29T16:12:32Z

Weird that GH wants to merge/rebase this with master. I did the rebase on top of fresh master, no other changes. 🤔

adlerjohn

Remove lockfile and update from master using the button if needed

Br1ght0ne · 2022-03-29T16:18:26Z

@adlerjohn Removed and cargo generate-lockfile-d a new one locally. Re button: merge commit or rebase? (no idea on the conventions)

iqdecay

Some remarks about the unsafe, and also I'm not sure about the error handling but this might come from a misunderstanding on my end :) Otherwise looks good!

iqdecay · 2022-03-29T16:20:38Z

fuels-signers/src/lib.rs

@@ -48,26 +46,26 @@ mod tests {
        let mut secret_seed = [0u8; 32];
        rng.fill_bytes(&mut secret_seed);

-        let secret =
-            SecretKey::from_slice(&secret_seed).expect("Failed to generate random secret!");
+        let secret = unsafe { SecretKey::from_bytes_unchecked(secret_seed) };


This should be both commented to explain why we are doing an unsafe block, and IMO we should figure out the proper way to make it non-unsafe. We can't really compromise memory safety (even in this trivial case) for something like signing.

The proper way would be via TryFrom.

However, from the lines above, looks like you are creating a new random key. Hence, in this case, the proper way would be to call Secret::random

fuels-signers/src/lib.rs

iqdecay · 2022-03-29T16:23:34Z

fuels-signers/src/util.rs

@@ -37,14 +36,10 @@ pub mod test_helpers {

        let secret_seed = rng.gen::<[u8; 32]>();

-        let secret =
-            SecretKey::from_slice(&secret_seed).expect("Failed to generate random secret!");
+        let secret = unsafe { SecretKey::from_bytes_unchecked(secret_seed) };


Same remark as before

iqdecay · 2022-03-29T16:23:51Z

fuels-signers/src/wallet.rs

@@ -32,8 +29,7 @@ use thiserror::Error;
 ///   let mut secret_seed = [0u8; 32];
 ///   rng.fill_bytes(&mut secret_seed);
 ///
-///   let secret =
-///       SecretKey::from_slice(&secret_seed).expect("Failed to generate random secret!");
+///   let secret = unsafe { SecretKey::from_bytes_unchecked(secret_seed) };


fuels-signers/src/wallet.rs

adlerjohn · 2022-03-29T18:16:06Z

merge commit or rebase?

Merge commit, so that reviewers know what's changed. On the topic of the lockfile, you need to remove because it's not supposed to be there: #179

Updating from master will also update your gitignore so it's not added again by accident.

resolved

Remove secp256k1 dep from fuels-signers Remove secp256k1 dep from fuels-signers

Remove unneeded Result

Br1ght0ne · 2022-04-12T15:13:11Z

Needs UTXO validation in send_transaction and transfer_coins_with_change. Working on it.

packages/fuels-signers/src/lib.rs

digorithm · 2022-05-03T22:03:32Z

@Br1ght0ne Please take a look at the changes I introduced at a5026c6.

Here's a quick explanation:

tx.id() hashes the transaction to get the transaction ID;
fuel-crypto's Message::new() hashes the bytes passed onto it.

Before, this is what you were doing:

let id = tx.id();
let sig = Signature::sign(&self.private_key, &Message::new(&id));

Effectively hashing the transaction bytes twice. This caused the recovered public key to be wrong because the Message was wrong (double hashed), causing a mismatch between the input coin's owner (correct) and the recovered public key (incorrect), which is the error you had been seeing all this time.

packages/fuels-signers/Cargo.toml

iqdecay

All my questions have been nicely addressed afai can see! lgtm!

Br1ght0ne marked this pull request as ready for review March 21, 2022 14:35

Br1ght0ne requested a review from digorithm as a code owner March 21, 2022 14:35

digorithm assigned Br1ght0ne Mar 21, 2022

digorithm added the enhancement New feature or request label Mar 21, 2022

adlerjohn marked this pull request as draft March 22, 2022 19:22

Br1ght0ne mentioned this pull request Mar 23, 2022

Add impl FromStr for Signature FuelLabs/fuel-crypto#16

Merged

Br1ght0ne force-pushed the oleksii/replace-vm-signatures-with-crypto branch from 77ef9c9 to d9fb22f Compare March 29, 2022 15:57

Br1ght0ne commented Mar 29, 2022

View reviewed changes

fuels-signers/src/lib.rs Outdated Show resolved Hide resolved

Br1ght0ne marked this pull request as ready for review March 29, 2022 16:11

Br1ght0ne requested a review from adlerjohn March 29, 2022 16:11

adlerjohn previously requested changes Mar 29, 2022

View reviewed changes

iqdecay suggested changes Mar 29, 2022

View reviewed changes

Br1ght0ne force-pushed the oleksii/replace-vm-signatures-with-crypto branch from e2dbb35 to bd63da9 Compare March 30, 2022 08:25

Br1ght0ne marked this pull request as draft March 31, 2022 09:07

Br1ght0ne added 2 commits April 12, 2022 18:03

Use signing/recovery from fuel_crypto in fuels_signers

f014958

Remove secp256k1 dep from fuels-signers Remove secp256k1 dep from fuels-signers

Fix tests and cargo lints

ebd17bd

Remove unneeded Result

Br1ght0ne force-pushed the oleksii/replace-vm-signatures-with-crypto branch from c546124 to ebd17bd Compare April 12, 2022 15:06

Voxelot reviewed Apr 13, 2022

View reviewed changes

packages/fuels-signers/src/lib.rs Show resolved Hide resolved

Br1ght0ne added 4 commits April 25, 2022 13:27

Re-export fuel-crypto

9d92af6

Merge branch 'master' into oleksii/replace-vm-signatures-with-crypto

85ecb52

Run clippy

a920ed4

Update fuels-test-helpers to use fuel-crypto

7717049

digorithm added 2 commits May 3, 2022 11:51

Sync with master

eef5279

Fix double tx id hashing

a5026c6

adlerjohn marked this pull request as ready for review May 4, 2022 16:17

adlerjohn reviewed May 4, 2022

View reviewed changes

packages/fuels-signers/Cargo.toml Show resolved Hide resolved

Nit

b975151

adlerjohn requested review from iqdecay and vlopes11 May 4, 2022 16:30

adlerjohn approved these changes May 4, 2022

View reviewed changes

iqdecay approved these changes May 5, 2022

View reviewed changes

digorithm merged commit adf81bd into master May 5, 2022

digorithm deleted the oleksii/replace-vm-signatures-with-crypto branch May 5, 2022 17:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use signing/recovery from fuel_crypto in fuels_signers #165

Use signing/recovery from fuel_crypto in fuels_signers #165

Br1ght0ne commented Mar 21, 2022 •

edited by adlerjohn

Loading

digorithm commented Mar 21, 2022

Br1ght0ne commented Mar 21, 2022

Br1ght0ne commented Mar 29, 2022

Br1ght0ne commented Mar 29, 2022

adlerjohn left a comment

Br1ght0ne commented Mar 29, 2022

iqdecay left a comment

iqdecay Mar 29, 2022

vlopes11 Apr 4, 2022

iqdecay Mar 29, 2022

iqdecay Mar 29, 2022

adlerjohn commented Mar 29, 2022

Br1ght0ne commented Apr 12, 2022 •

edited

Loading

digorithm commented May 3, 2022 •

edited by adlerjohn

Loading

iqdecay left a comment

Use signing/recovery from fuel_crypto in fuels_signers #165

Use signing/recovery from fuel_crypto in fuels_signers #165

Conversation

Br1ght0ne commented Mar 21, 2022 • edited by adlerjohn Loading

digorithm commented Mar 21, 2022

Br1ght0ne commented Mar 21, 2022

Br1ght0ne commented Mar 29, 2022

Br1ght0ne commented Mar 29, 2022

adlerjohn left a comment

Choose a reason for hiding this comment

Br1ght0ne commented Mar 29, 2022

iqdecay left a comment

Choose a reason for hiding this comment

iqdecay Mar 29, 2022

Choose a reason for hiding this comment

vlopes11 Apr 4, 2022

Choose a reason for hiding this comment

iqdecay Mar 29, 2022

Choose a reason for hiding this comment

iqdecay Mar 29, 2022

Choose a reason for hiding this comment

adlerjohn commented Mar 29, 2022

Br1ght0ne commented Apr 12, 2022 • edited Loading

digorithm commented May 3, 2022 • edited by adlerjohn Loading

iqdecay left a comment

Choose a reason for hiding this comment

Br1ght0ne commented Mar 21, 2022 •

edited by adlerjohn

Loading

Br1ght0ne commented Apr 12, 2022 •

edited

Loading

digorithm commented May 3, 2022 •

edited by adlerjohn

Loading