Remove store usage related to ledger from wallet.js - #2422
Conversation
|
Your Render PR Server URL is https://near-wallet-pr-2422.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-c7q5p4n6d9kt3cffucsg. |
marcinbodnar
changed the title
|
@stefanopepe
|
andy-haynes
left a comment
andy-haynes
left a comment
There was a problem hiding this comment.
Looking good, just a few comments inline.
| throw e; | ||
| }, | ||
| () => { | ||
| dispatch(checkAndHideLedgerModal()); |
There was a problem hiding this comment.
Should this have an await? I see that the createAsyncThunk call that creates checkAndHideLedgerModal has synchronous logic in its payloadCreator argument but since that function is declared async it will still return a Promise. If an await were added to that payloadCreator body later on, an await would need to be added here anyway to avoid fire-and-forgetting a Promise that needed to be waited on. Even worse would be if an error were thrown from checkAndHideLedgerModal then this would potentially lead to an unhandled Promise rejection.
Alternatively we could omit createAsyncThunk for synchronous thunks if we don't think they'll be extended in the future and it doesn't introduce inconsistencies in our action handling.
| async ({ path } = {}, { dispatch }) => { | ||
| const { createLedgerU2FClient } = await import('../../../utils/ledger.js'); | ||
| const client = await createLedgerU2FClient(); | ||
| dispatch(handleShowLedgerModal({ show: true })).unwrap(); |
There was a problem hiding this comment.
Is the .unwrap() necessary here if we're not interested in the return value?
| `${SLICE_NAME}/handleShowLedgerModal`, | ||
| async ({ show }, { dispatch, getState }) => { | ||
| const actionStatus = selectStatusActionStatus(getState()); | ||
| const actions = Object.keys(actionStatus).filter((action) => actionStatus[action]?.pending === true); |
There was a problem hiding this comment.
Can pending be truthy without being explicitly true? Just curious if the explicit true check is required here
|
@marcinbodnar are these changes still relevant after the new Ledger connection implementation? |
@Patrick1904 yes, but I will probably have to do it again because there are too much conflicts now. |
3 participants
With this PR we are removing all usage of
storerelated to ledger from wallet.js.