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

Add set account passphrase #2664

Merged
merged 31 commits into from Apr 23, 2021
Merged

Conversation

vctt94
Copy link
Member

@vctt94 vctt94 commented Sep 9, 2020

contains db upgrade -- backup db before testing

This PR depends on #2665 and on decred/dcrwallet#1823

This PR depends on decred/dcrwallet#1979

depends on #3337

MAKE A DB BACKUP BEFORE TESTING, AS IT WILL SET ALL ACCOUNTS YOUR WALLET PASSPHRASE

This PR adds a new step into the setup wallet machine, which sets all account's passphrase as the wallet passhrase.

Right now what I changed to use lock/unlock account instead of wallet:

  • send tx;
  • purchase ticket and auto buyer (legacy and non legacy);
  • privacy - running the mixer.

The rest of the wallet operations still are with old functionalities

@vctt94 vctt94 changed the title Start adding set account passphrase [wip]Start adding set account passphrase Sep 9, 2020
@vctt94 vctt94 force-pushed the add-per-acct-encryption branch 2 times, most recently from 88f8508 to 8cbf22e Compare September 11, 2020 15:20
@vctt94 vctt94 changed the title [wip]Start adding set account passphrase Add set account passphrase Sep 12, 2020
@alexlyp
Copy link
Member

alexlyp commented Sep 22, 2020

Can rebase this now that 2665 is in

@alexlyp
Copy link
Member

alexlyp commented Sep 22, 2020

Hrm looks like there's another rebase that's needed, sorry

@vctt94 vctt94 force-pushed the add-per-acct-encryption branch 2 times, most recently from ed69274 to 2250d5d Compare September 28, 2020 18:10
@vctt94 vctt94 changed the title Add set account passphrase [wip] Add set account passphrase Jan 28, 2021
@vctt94 vctt94 changed the title [wip] Add set account passphrase Add set account passphrase Feb 3, 2021
@alexlyp
Copy link
Member

alexlyp commented Feb 19, 2021

Now that 1.6.1 is pretty much done, a rebase is due on this one.

Comment on lines 230 to 246
try {
const acctNumber = acct.encrypted ? acct.value : null;
const error = await dispatch(unlockWalletOrAcct(passphrase, acctNumber));
if (error) {
return dispatch({ error, type: SIGNTX_FAILED });
}
const signTransactionResponse = await wallet.signTransaction(
sel.walletService(getState()),
rawTx
);
dispatch({
signTransactionResponse: signTransactionResponse,
type: SIGNTX_SUCCESS
});
dispatch(
publishTransactionAttempt(signTransactionResponse.getTransaction())
);
await dispatch(lockWalletOrAcct(acctNumber));
} catch(error) {
dispatch({ error, type: SIGNTX_FAILED });
}
Copy link
Member

Choose a reason for hiding this comment

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

This whole thing can leave the account unlocked if signTransaction fails (since that could trigger an exception).

In general, the unlockWalletOrAcct(); doSomething(); lockWalletOrAcct() pattern is going to be tricky to get (and maintain) correct at every instance an account unlocking can happen.

I suggest writing generic unlock/lock action, that takes as argument an async function and then unconditionally performs the unlock. Something like:

export const performWithUnlockedAccount = async (passphrase, acct, fn) => (dispatch) => {
  // TODO: error handling, etc
  await dispatch(unlockWalletOrAcct(passphrase, acctNumber));
  let fnError;
  let res;
  try {
    res = await fn();
  } catch (error) {
    fnError = error
  }
  await dispatch(lockWalletOrAcct(acctNumber));
  if (fnError) throw fnError;
  return res;
}

const { ticketBuyerService } = getState().grpc;
dispatch({ ticketBuyerConfig, type: STARTTICKETBUYERV2_ATTEMPT });
const accountNum = account.encrypted ? account.value : null;
const error = await dispatch(unlockWalletOrAcct(passphrase, accountNum));
Copy link
Member

Choose a reason for hiding this comment

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

If I send a generic tx from the same account the ticket buyer is purchasing tickets from, will the lockWalletOrAcct() call at the end of transaction interfere with the ticketbuyer?

Copy link
Member Author

Choose a reason for hiding this comment

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

Now it is not possible to do other actions while mixer or auto buyer is running, anymore.

@vctt94 vctt94 changed the title Add set account passphrase [wip] Add set account passphrase Mar 23, 2021
@vctt94 vctt94 force-pushed the add-per-acct-encryption branch 7 times, most recently from 82bed7c to 590c20d Compare April 8, 2021 18:25
Copy link
Member

@amass01 amass01 left a comment

Choose a reason for hiding this comment

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

Code looks good overall, left some minors.

import { PassphraseModalButton, InvisibleButton } from "buttons";
import styles from "./SettingAccountsPassphrase.module.css";

export default ({
Copy link
Member

Choose a reason for hiding this comment

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

pls use const SettingAccountsPasspharse.
Also, I feel that SetAccountsPasspharse might be a better name for this component.

@@ -461,9 +443,11 @@ export const setVspdAgendaChoices = (
export const unlockWallet = (walletService, passphrase) =>
new Promise((resolve, reject) => {
const unlockReq = new api.UnlockWalletRequest();
console.log(console.log(passphrase));
Copy link
Member

Choose a reason for hiding this comment

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

console

unlockReq.setPassphrase(new Uint8Array(Buffer.from(passphrase)));
// Unlock wallet so we can call the request.
walletService.unlockWallet(unlockReq, (error) => {
console.log(error);
Copy link
Member

Choose a reason for hiding this comment

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

console

resolve(response);
});
});

Copy link
Member

Choose a reason for hiding this comment

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

unnecessary empty line

@alexlyp
Copy link
Member

alexlyp commented Apr 8, 2021

On the Account passphrase page during start up with Skip and X button are shown. These should all be hidden since we are going to force the upgrade.

@alexlyp
Copy link
Member

alexlyp commented Apr 8, 2021

I'll work on some copy to use on startup page.

dispatch(
publishTransactionAttempt(signTransactionResponse.getTransaction())
);
await dispatch(lockWalletOrAcct(acctNumber));
Copy link
Member

Choose a reason for hiding this comment

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

Is this still needed now that signing happens inside unlockAcctAndExecFn?

type: CREATE_UNSIGNEDTICKETS_SUCCESS
});
}
await dispatch(lockWalletOrAcct(accountNum));
Copy link
Member

Choose a reason for hiding this comment

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

Still needed?

export const UNLOCKACCTORWALLET_FAILED = "UNLOCKACCTORWALLET_FAILED";
export const UNLOCKACCTORWALLET_SUCCESS = "UNLOCKACCTORWALLET_SUCCESS";

// unlockAcctAndExecFn unlocks the account and perform some acction. Locks the
Copy link
Member

Choose a reason for hiding this comment

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

perform => performs
acction => action

Comment on lines 1056 to 1058
if (fnError !== null) {
throw fnError;
}
Copy link
Member

Choose a reason for hiding this comment

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

This will leave the account unlocked if fnError is filled in the block above. This needs to happen after the wallet is locked (if leaveUnlock is false).

Copy link
Member Author

Choose a reason for hiding this comment

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

true, good catch.

@alexlyp alexlyp merged commit 2ecaffe into decred:master Apr 23, 2021
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.

None yet

4 participants