-
Notifications
You must be signed in to change notification settings - Fork 45
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
Remove global walletId and index accounts by address #1019
Conversation
MegaLinter status: ✅ SUCCESS
See errors details in artifact MegaLinter reports on CI Job page |
Codecov Report
@@ Coverage Diff @@
## master #1019 +/- ##
==========================================
- Coverage 88.57% 88.55% -0.02%
==========================================
Files 101 101
Lines 1750 1747 -3
Branches 405 403 -2
==========================================
- Hits 1550 1547 -3
Misses 200 200
Flags with carried forward coverage won't be shown. Click here to find out more.
|
selectedWallet?: number | ||
wallets: { [id: number]: Wallet } | ||
selectedWallet?: string | ||
wallets: { [address: string]: Wallet } |
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.
Instead of using { [address: string]: Wallet }
I find the Record<string, Wallet>
form easier to read.
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 the key name address
is helpful here. Too many things are a string
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.
That's true. In fact it's so true that I would consider adding export type Address = string
somewhere, and then consistently using Address
instead of string
wherever we are talking about addresses. (But that's a story of a different PR.)
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.
Yeah :D would we consistently use it tho? maybe :D
If we add it, we should alias it like StringifiedBigInt, or make it branded
https://github.com/oasisprotocol/oasis-wallet-web/blob/8be6fcb13735fa5dee04c8be75918078e5b6ef56/src/types/StringifiedBigInt.ts#L1-L3
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.
Reviewed. LGTM.
(Almost feels like it should have been written like this from the start.)
dc6772e
to
88e5c8f
Compare
src/app/state/wallet/index.ts
Outdated
}, | ||
addWallet(state, action: PayloadAction<AddWalletPayload>) {}, | ||
walletOpened(state, action: PayloadAction<Wallet>) { | ||
const newWallet = action.payload | ||
state.wallets[newWallet.id] = Object.assign({}, newWallet) | ||
state.wallets[newWallet.address] = { ...newWallet } |
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.
hmm I thought immer
takes care of immutability and there is no need to use spread operator or Object.assign when assigning an object to state.
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.
Alright I'll risk it :D
88e5c8f
to
218916a
Compare
218916a
to
0186a14
Compare
Motivation:
let walletId = 0
is annoying to add to persisted state