Giv/8 one address by givp · Pull Request #21 · polymorpher/one-wallet

givp · 2021-06-30T00:46:44Z

I've tested with both one and eth addresses and they work interchangeably everywhere.

polymorpher · 2021-06-30T01:15:49Z

      return
    }
+    // Check for one address
+    let verifiedLastResortAddress = lastResortAddress


Can we makes this a utility in util.js with boolean return value? Also, it should show some feedback to the user (e.g. message.error) at appropriate time (e.g. when full length is reached) when the address is incorrect. It should also give some visual signal when the full length is yet to be reached.

polymorpher · 2021-06-30T01:16:06Z

    const neighbor = neighbors[0]
+
+    // Check for one address
+    let verifiedTransferTo = transferTo


Can be merged with above using utility function

For sure, too much duplication. Will fix.

polymorpher · 2021-06-30T01:24:05Z

Screen.Recording.2021-06-29.at.6.19.17.PM.mov

givp · 2021-06-30T04:33:57Z

@polymorpher please check again. I've moved the address checker to utils for both wallet creation and transfers. Added a message for the user when address check fails.

Also, I noticed the transferAmount check wasn't working since it's a BN so I fixed that here.

polymorpher · 2021-06-30T11:09:11Z

        lifespan: duration / WalletConstants.interval,
        slotSize,
-        lastResortAddress,
+        validLastResortAddress,


This won't work because it would change the key of the object.

polymorpher · 2021-06-30T11:09:18Z

        duration,
        effectiveTime,
-        lastResortAddress,
+        validLastResortAddress,


Same issue as above

polymorpher · 2021-06-30T11:16:08Z

          <Space direction='vertical' size='small'>
            <Hint>Wallet Address</Hint>
-            <Text>{address}</Text>
+            <Text>{address && getAddress(address).bech32}</Text>


The implementation of getAddress seems to be quite slow. The first thing it does seems to be a regex matching https://github.com/harmony-one/sdk/blob/3c7061887f25c6ba8def5af05672cdc4f44eb2dc/packages/harmony-utils/src/validators.ts#L6 , followed by multiple passes of the 20 bytes array and many string operations and comparisons. It may be too slow to be in the rendering loop. We might need another state variable for it, watched by useEffect and only converted when it is at full length.

polymorpher · 2021-06-30T11:17:34Z

  const history = useHistory()
  const location = useLocation()
  const { address, name } = wallet
+  const oneAddress = getAddress(address).bech32


Same as above. Another approach is to useMemo, or to implement a custom hook (e.g. useAddress)

polymorpher · 2021-06-30T11:26:30Z

  const price = useSelector(state => state.wallet.price)
  const { formatted, fiatFormatted } = util.computeBalance(balance, price)
  const { dailyLimit, lastResortAddress } = wallet
+  const oneLastResort = lastResortAddress && getAddress(lastResortAddress).bech32


Same as above

polymorpher · 2021-06-30T11:26:36Z

  const wallets = useSelector(state => state.wallet.wallets)
  const match = useRouteMatch(Paths.show)
  const { address, action } = match ? match.params : {}
+  const oneAddress = getAddress(address).bech32


Same as above

polymorpher · 2021-06-30T11:39:26Z

    return { balance, formatted, fiat, fiatFormatted, valid: true }
+  },
+
+  validateAddress: (address) => {


I see error message is prompted in this function instead of at each component themselves. And this function actually convert the address to ETH form, not just validate an address. Perhaps it should be renamed to normalizeAddress or something similar, and the UI prompt should be moved out to another function (e.g. promptBadAddress)

polymorpher · 2021-06-30T11:40:29Z

    const neighbor = neighbors[0]
+
+    // Ensure valid address for both 0x and one1 formats
+    const validAddress = util.validateAddress(transferTo)


normalizedAddress?

polymorpher · 2021-06-30T11:40:55Z

    }
+
+    // Ensure valid address for both 0x and one1 formats
+    const validLastResortAddress = util.validateAddress(lastResortAddress)


normalizedAddress?

polymorpher · 2021-06-30T11:41:54Z

+    // Ensure valid address for both 0x and one1 formats
+    const validLastResortAddress = util.validateAddress(lastResortAddress)
+    if (!validLastResortAddress) {
+      return


It would be quite hard to keep tracking error handling if the message prompt is absorbed in above function and its name does not suggest it does handle errors.

givp · 2021-06-30T15:22:31Z

@polymorpher sorry for the confusion. util.validateAddress name is confusing as you suggested. It doesn't just validate, it confirms the format of one1 and 0x addresses but always returns ETH format. So normalizedAddress makes more sense. I'll change that. But the relayer calls are not affected. I'm still sending 0x addresses for all transactions, even if the user enters an one1 address.

In terms of console errors, I will add them in the function but was trying to avoid console noise .

givp · 2021-06-30T15:37:37Z

This should be in a good place to merge. I'm going to revisit the getAddress speed issue once I've wrapped up some of the other tasks.

polymorpher · 2021-06-30T19:23:35Z

        lifespan: duration / WalletConstants.interval,
        slotSize,
-        lastResortAddress,
+        normalizedAddress,


This is still an issue. The expected object key is lastResortAddress

polymorpher · 2021-06-30T19:23:50Z

        duration,
        effectiveTime,
-        lastResortAddress,
+        normalizedAddress,


same as above

Ah apologies, fixing.

# Conflicts: # code/client/src/util.js

…and utilities

polymorpher · 2021-07-01T01:48:27Z

I made a quick update on separation of error handling logic, and simplified the normalization: d9a92a5

givp added 2 commits June 29, 2021 17:05

Updated list and show one1 addresses

580ff44

Migrated to one1 address

71ddf9a

givp requested a review from polymorpher June 30, 2021 00:46

givp linked an issue Jun 30, 2021 that may be closed by this pull request

2. Support one1.... style address as recovery address #8

Closed

polymorpher reviewed Jun 30, 2021

View reviewed changes

givp added 2 commits June 29, 2021 20:37

Merge branch 'master' into giv/8-one-address

bf26ce6

Moved address validator to utils + fixed transferAmount checker

e7dc8ca

polymorpher reviewed Jun 30, 2021

View reviewed changes

Renaming normalizeAddress function

6bbcc98

polymorpher reviewed Jun 30, 2021

View reviewed changes

givp and others added 3 commits June 30, 2021 12:28

lastResortAddress param fix

88b4866

Merge branch 'master' into giv/8-one-address

54bf866

# Conflicts: # code/client/src/util.js

simpler address normalization. separation of logic on error handling …

d9a92a5

…and utilities

polymorpher merged commit 88ac339 into master Jul 1, 2021

Conversation

givp commented Jun 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polymorpher commented Jun 30, 2021

Uh oh!

givp commented Jun 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polymorpher Jun 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

givp commented Jun 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

givp commented Jun 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polymorpher commented Jul 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

polymorpher Jun 30, 2021 •

edited

Loading

givp commented Jun 30, 2021 •

edited

Loading