feat(wallet-ui): provision new wallet #6289
Conversation
samsiegart
force-pushed
the
provision-wallet-ui
branch
from
ff0ad4c to
1ff7a4c
Compare
| return; | ||
| } | ||
|
|
||
| setError(''); |
There was a problem hiding this comment.
why is this necessary? seems an odd place for it. do you want to reset the error when the user initiates a new action?
why use empty string instead of null or undefined?
There was a problem hiding this comment.
Yea we want to reset the error when the user initiates a new action.
I sometimes get in the habit of using empty string because certain input elements don't play well with null, but no reason otherwise. Typically I never set things to undefined explicitly but maybe that's okay as well.
There was a problem hiding this comment.
Yea we want to reset the error when the user initiates a new action.
Then please put it where they initiate the action. Either at the top of this function or sooner.
certain input elements don't play well with null, but no reason otherwise
Please use null for "no error". Empty string could be an error that just doesn't have any description. If there are elements where this causes problems I'd like to fix them
There was a problem hiding this comment.
Changed to use null instead of empty string
I didn't want to have two setErrors in the case of the early return above. I just split this into another function to make it more readable if that's any better?
| <b>Wallet Address:</b> {address} | ||
| </DialogContentText> | ||
| <DialogContentText sx={{ pt: 2 }}> | ||
| There is no smart wallet provisioned for this address yet. A fee |
There was a problem hiding this comment.
if we're not going to read it from params, at least make it a const. should have a comment indicating the tech debt and why
There was a problem hiding this comment.
Made a const and TODO
| const { href, smartConnectionMethod } = connectionConfig; | ||
| let publicAddress; | ||
| if ( | ||
| smartConnectionMethod === SmartConnectionMethod.KEPLR && | ||
| keplrConnection | ||
| ) { | ||
| publicAddress = keplrConnection.address; | ||
| } else if (smartConnectionMethod === SmartConnectionMethod.READ_ONLY) { | ||
| publicAddress = connectionConfig.publicAddress; | ||
| } |
There was a problem hiding this comment.
consider a helper method so this is one line here assigning to a const
| value: { | ||
| address: b64address, | ||
| nickname: 'my wallet', | ||
| // XXX dry with PowerFlags const |
There was a problem hiding this comment.
One concern I have is when we import these packages just to use a const or type etc, we increase the bundle size by a lot. Maybe it's better to just leave it?
There was a problem hiding this comment.
Doesn't the bundler have tree shaking? If not please make a ticket to get that working and include it in the comment here as to why these aren't imported yet
There was a problem hiding this comment.
Looks like it does have tree shaking yes. Importing this only increased the bundle size by ~4kB. Still the bundle size is getting up there, so I created a separate issue for that #6291
You mean just squash? I'm not sure how else. |
I was thinking an interactive rebase with renaming. E.g. |
samsiegart
force-pushed
the
provision-wallet-ui
branch
from
b67f22c to
db9966c
Compare
Ah okay, yea just tried to clean up the messages now |
samsiegart
added
automerge:squash
samsiegart
force-pushed
the
provision-wallet-ui
branch
from
6134bb7 to
b2f71fd
Compare
fixes: #6211