feat: add the sign feature #24
Conversation
kayagokalp
force-pushed
the
kayagokalp/7
branch
from
89cf37d
to
b9e582f
Compare
|
I am converting this to draft, because just now I managed to sign a transaction and while doing so I had to make some changes. I will update the description while pushing them. |
Pull request was converted to draft
|
This is ready to go! I added the plain address section to the The newly added I can also improve the CLI experience (with some suggestions 😅) in this PR too, but if you prefer reaching a point where we can complete the whole signing process first and iterate over it for better UX I am also down for that too. |
src/account.rs
Outdated
| println!("Wallet address: {}", wallet.address()); | ||
| println!("Wallet plain address: {}", wallet.address().hash()); |
There was a problem hiding this comment.
Is the plain address the hash of the pubkey? I don't think that should be printed out by default. Regular end-users should be using bech32 addresses exclusively. Pubkey hash should be something users need to purposefully seek out as advanced users.
There was a problem hiding this comment.
I added the hash of the pubkey, since it is useful/needed while testing in local as (if I am not mistaken) chain config requires that for funding the wallet with initial coins. But this is indeed something an advanced user would use. Maybe we can introduce a flag to account command in a future PR such that executing something like forc-wallet account <account_index> --hashed would return this. If this sounds good I can create an issue
src/list.rs
Outdated
| "[{}] {} 0x{}", | ||
| index, | ||
| address, | ||
| Bech32Address::from_str(&address)?.hash() |
src/sign.rs
Outdated
| }; | ||
| let tx_id = Bytes32::from_str(id).map_err(|e| anyhow!("{}", e))?; | ||
| let secret_key = derive_account_with_index(&wallet_path, account_index)?; | ||
| let message = unsafe { Message::from_bytes_unchecked(*tx_id) }; |
There was a problem hiding this comment.
For clarity, this should be called message hash?
There was a problem hiding this comment.
Just tested this locally with your forc deploy branch and it appears to be working nicely!
| )?; | ||
| let phrase_recovered = eth_keystore::decrypt_key(path.join(".wallet"), password)?; | ||
| let phrase = String::from_utf8(phrase_recovered)?; | ||
| let derive_path = format!("m/44'/1179993420'/{}'/0/0", account_index); |
There was a problem hiding this comment.
Perhaps we should be using an upstream const and/or function for constructing this path? E.g. at least a wallet_account_derive_path function or similar. It's probably fine for now, but I've opened an issue to follow-up: #28.
About this PR
closes #7.
This PR introduces a new command
signto theforc-wallet. Currentlysignaccepts 2 parameters:In this first iteration of development (where we are trying to enable our users to sign deployment transactions) The usual signing process while deploying a contract currently looks like the following:
forc-deploywill ask the user to input the address of the wallet they are going to be signing with. To get that the user can either run theforc-wallet listand look under theaddresssection of the desired account or runforc-wallet account <account_index>to get the address. This reports a transaction id and waits for the signature.forc-wallet sign <transaction_id> <account_index>andforc-walletasks for the password for the wallet as we do need to decrypt it before signing. If the password is correct, the signature is generated and reported back to the user.forc-deployis waiting for the signature to be inserted, once the user inserts the generated signature from theforc-walletit adds it to the witnesses and submits the transaction.All the required functionality from the wallet's end is ready with this PR and once FuelLabs/sway#2629 is merged
forc-deploywill also behave as described.