Feat/new-address-modal #661

amovfx · 2023-01-24T01:19:57Z

Hey Jamal,
This is new-address-modal for your to review the inital work flow.

I still have to do tests and some polish on the code to bring it into line with what I learned on the last pr, but it's functional and ready for critique.

amovfx · 2023-01-24T01:23:41Z

Oh man I think I messed up the rebase, might want to hold off.

Just to clarify, I should rebase this branch onto feat/add-taro correct?

jamaljsr · 2023-01-24T12:52:47Z

Just to clarify, I should rebase this branch onto feat/add-taro correct?

Yes, that is correct. You should drop any commits that are already in that branch.

Fixed error displays on new address modal

amovfx · 2023-01-24T19:17:22Z

This has been a very good learning experience. I think I've fixed the rebasing.
This is good to review preliminary workflow and UX. If you want to review the code, that would be cool, but I plan on doing a pass over to get into line with what I've learned from your previous notes.

jamaljsr · 2023-01-24T20:27:49Z

Thanks for the updates. I'll do an initial UX review by tomorrow.

Thanks for continuing to work on these updates.

amovfx · 2023-01-24T22:22:59Z

You're welcome. Thanks for taking the time to work with me on this.

jamaljsr · 2023-01-26T04:41:06Z

Overall it works good on the UX side. I do have some suggestions for improvement.

I don't think it's a good idea to instantly generate a new address every time one of the input fields changes. This is going to cause many addresses to be created in the taro db. For example, if I change the amount field from 1 to 1000, it will create 3 addresses, 2 of which I do not need. These addresses are persisted by taro and can be viewed via tarocli addrs query.
Rename "Import from Taro Node" to "Choose asset from Taro node". The term "Import" is a bit confusing.
Instead of displaying the address in the initial form, I think it would be better to keep the flow similar to how invoices are created. Initially display the fields that the user can modify. When they click the Generate button, the form is hidden and the address is displayed along with a "Copy & Close" button.
When you copy the address, the message that pops up at the top of the screen shows the full address and looks pretty bad. This should be ellipsed so it's not so wide.
In the Actions sidebar, the "New Address" button should be full width, not half width.

-Address is generated on submission and displayed on a result modal like create invoice. -Renamed collapsible "Import from Taro Node" to "Choose asset from Taro node" -Copied address message displays at top after explicit copy. -Actions tab: New Address and Mint Asset buttons now take up 100% width.

amovfx · 2023-01-27T18:11:48Z

Excellent feedback, this is working way better. If you are happy with this, I will do a final pass on the code to attempt to bring it up to the standard of your previous reviews.

jamaljsr · 2023-01-28T19:51:56Z

Just tested 77f2756. This feels much better. Just a couple more comments about the latest updates.

After clicking Generate and displaying the address, the modal's footer should no longer be displayed. It allows the user to click Generate button again, but it doesn't actually do anything.
After choosing a token from another node, the Amount field is updated to contain the full amount. I'm not sure most people would want to drain the sending node's wallet in one payment. I think it'll be more common to want to send a bunch of smaller payments. So maybe it makes more sense to set this to a small number, like 10, and don't change it when an asset is selected.

On a side note, as I've been playing more with generating addresses, I've noticed a pain point. When minting a token I keep forgetting to change the amount. I realize it when I go to create an address. Then I need to go back and create a new asset. I think we should increase the default amount in the Mint Asset modal to 1000.

Let me know when you are ready for a code review.

Removed footer on new address modal once taro address is generated. Mint asset modal amount defaults to 1000 Amount to send has a new defualt of 10 or the lower balance amount. Amount is disabled if balance is a collecitble.

amovfx · 2023-01-29T01:11:36Z

This is really shaping up. Thanks for the feedback.

Did a pass over the code to get it in line with previous feedback.

amovfx · 2023-01-29T23:03:02Z

This is ready for review.
Something that bothers me, I don't really have a way to quickly validate a taro address off the top of my head.
I think it would be cool to have a regex function that checks to make sure that a genesis info is valid. Are all taro addresses the same length?

jamaljsr · 2023-01-30T02:44:59Z

Cool. I'll go through the code tomorrow.

Regarding the data, I honestly think its better if Polar is lenient when it comes to validation. If we get too strict and tightly coupled to the implementations, it increases the chances of breaking changes with future node releases. I prefer to minimize the need to ship a new Polar release just to support a new version of a node. I am fine with not validating the data user's enter. We just need to display the error that the nodes will respond with. It also gives devs more flexibility for testing without Polar getting in the way.

jamaljsr

Took a first pass on the code. It's mostly all good. Just some suggestions to simplify the components.

Also, be sure to run yarn lint to catch the couple linter warnings.

electron/tarod/tarodProxyServer.ts

src/components/designer/taro/actions/NewAddressModal.tsx

src/components/common/form/TaroNodeSelect.tsx

src/components/designer/taro/actions/NewAddressButton.tsx

src/components/designer/taro/actions/NewAddressModal.tsx

src/i18n/locales/en-US.json

src/store/models/taro.ts

Removed debug statement from tarodProxyServer. Added comment to NewAddressModal syncChart call. Refactored Collapse and TaroAssetSelect to TaroDataSelect. NewAddressButton label texts is no consistent with prior buttons. NewAddressModal sets better defaults. getNewAddress removed redudant string cast. Cleaned up tests and linting.

amovfx · 2023-01-31T03:31:52Z

I ended up doing a pretty big refactor. The collapse panel is gone, I thought it was a bit redundant and TaroNodeSelect and TaroAssetSelect are collapsed into a single component. , but if you think it's a good idea to hide the select balance functionality behind it, I'll put it back.

Optimization of the loop that builds the options data of the select form.

jamaljsr

This is definitely a step in the right direction. The NewAddressModal component looks much better. I have more follow-up comments based on your latest changes.

src/components/designer/taro/actions/MintAssetButton.tsx

src/components/designer/taro/actions/NewAddressButton.tsx

src/components/common/form/TaroDataSelect.tsx

src/components/designer/taro/actions/NewAddressModal.tsx

jamaljsr · 2023-02-01T04:37:40Z

src/components/designer/taro/actions/NewAddressModal.tsx

+            <InputNumber
+              onChange={v => setSelectedAmount(v?.valueOf()?.toString() as string)}
+              min={'1'}
+              disabled={selectedBalance?.type === PTARO.TARO_ASSET_TYPE.COLLECTIBLE}
+            />


There's a few things that could be improved here. Take a look at how <InputNumber> is used throughout the code base and try to mirror those.

For example, here's how it's implemented in the OpenChannelModal component:

<InputNumber<number> disabled={openChanAsync.loading} formatter={v => `${v}`.replace(/\B(?=(\d{3})+(?!\d))/g, ',')} parser={v => parseInt(`${v}`.replace(/(undefined|,*)/g, ''))} style={{ width: '100%' }} onChange={v => setSelectedSats(v as number)} />

I seem to be using this in everything that I have done so far. What do you think of me moving this out to its own component in a refactor? I'm thinking something like components/common/form/AmountInput.tsx?

src/components/designer/taro/actions/NewAddressModal.tsx

jamaljsr

Alright. This looks good to go now. Thanks for iterating on this and getting the feature complete. 👌

amovfx force-pushed the feat/new-address-modal branch from c357c51 to 9f569db Compare January 24, 2023 18:48

fix(new-address-modal): squish commits

6b9bdb9

amovfx force-pushed the feat/new-address-modal branch from 9f569db to 6b9bdb9 Compare January 24, 2023 19:05

fix(new-address-modal): tidy up modal

a6a8889

Fixed error displays on new address modal

fix(workflow): feedback

501bf6f

Removed footer on new address modal once taro address is generated. Mint asset modal amount defaults to 1000 Amount to send has a new defualt of 10 or the lower balance amount. Amount is disabled if balance is a collecitble.

fix(modal): reviewed code

53273a6

Did a pass over the code to get it in line with previous feedback.

jamaljsr reviewed Jan 30, 2023

View reviewed changes

Andrew Oseen added 3 commits January 30, 2023 20:34

fix(new-address-modal): pruned arg for removed parameter

f920683

fix(TaroDataSelect): optimizations

36ab485

Optimization of the loop that builds the options data of the select form.

fix(TaroDataSelect): added missing type

7f79a5d

jamaljsr reviewed Feb 1, 2023

View reviewed changes

fix(review): second review notes

30bdee5

amovfx marked this pull request as ready for review February 1, 2023 19:14

fix(linting): removed linting errors

a71fd8c

jamaljsr approved these changes Feb 2, 2023

View reviewed changes

jamaljsr merged commit 8fbb7b7 into jamaljsr:feat/add-taro Feb 2, 2023

Feat/new-address-modal #661

Feat/new-address-modal #661

Uh oh!

Conversation

amovfx commented Jan 24, 2023

Uh oh!

amovfx commented Jan 24, 2023

Uh oh!

jamaljsr commented Jan 24, 2023

Uh oh!

amovfx commented Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamaljsr commented Jan 24, 2023

Uh oh!

amovfx commented Jan 24, 2023

Uh oh!

jamaljsr commented Jan 26, 2023

Uh oh!

amovfx commented Jan 27, 2023

Uh oh!

jamaljsr commented Jan 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amovfx commented Jan 29, 2023

Uh oh!

amovfx commented Jan 29, 2023

Uh oh!

jamaljsr commented Jan 30, 2023

Uh oh!

jamaljsr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amovfx commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamaljsr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jamaljsr Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

amovfx Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

amovfx Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jamaljsr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amovfx commented Jan 24, 2023 •

edited

Loading

jamaljsr commented Jan 28, 2023 •

edited

Loading

amovfx commented Jan 31, 2023 •

edited

Loading