-
Notifications
You must be signed in to change notification settings - Fork 43
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 encryption flow to redux #59
Conversation
@@ -61,7 +95,7 @@ export const selectTransactionItems = createSelector(selectTransactionsById, byI | |||
txid, | |||
date: tx.timestamp ? new Date(Number(tx.timestamp) * 1000) : null, | |||
amount, | |||
fee: amount < 0 ? (-1 * tx.fee) / append.length : 0, | |||
fee: amount < 0 ? -1 * tx.fee / append.length : 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.
This should not be here, I will check my local commits and try to find why this change is appearing.
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.
Guessing it's from Prettier.
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.
A few minor comments.
src/redux/actions/wallet.js
Outdated
type: ACTIONS.WALLET_LOCK_START, | ||
}); | ||
|
||
Lbry.wallet_lock().then(() => { |
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.
What if this command fails?
src/redux/reducers/wallet.js
Outdated
walletDecryptPending: false, | ||
walletDecryptSucceded: null, | ||
walletUnlockPending: false, | ||
walletUnlockSucceded: null, |
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.
There should probably be an error attribute here too. Not sure what the daemon returns but it would be nice to know if there was an error decrypting or some unrelated error.
src/redux/selectors/wallet.js
Outdated
@@ -2,6 +2,40 @@ import { createSelector } from 'reselect'; | |||
|
|||
export const selectState = state => state.wallet || {}; | |||
|
|||
export const selectWalletState = selectState; | |||
|
|||
export const selectWalletIsEncrypted = state => selectWalletState(state).walletIsEncrypted === true; |
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 would use createSelector
here like you did below.
walletLockSucceded: false, | ||
walletLockResult: action.result, | ||
}); | ||
|
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.
We should add Flow types to this.
@@ -6,6 +7,36 @@ const buildDraftTransaction = () => ({ | |||
address: undefined, | |||
}); | |||
|
|||
// TODO: Split into common success and failure types | |||
// See details in https://github.com/lbryio/lbry/issues/1307 | |||
type ActionResult = { |
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.
Maybe EncryptActionResult
?
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.
Likely it'll split into multiple at some point.
We don't have any predictable pattern at the moment and this only contains result
and could apply to any other action, I can change it but I still think we'll end up with another common pattern.
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.
👍
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 think we should rename that type to specify it's related to encryption
See #36
lbryio/lbry-desktop#1097 will be updated tomorrow with a dependency on these changes.