correction in contacts screen to know when it is neo 3 or neoLegacy #2140

endkeyCoder · 2021-06-29T16:00:58Z

…2138 #2137

What current issue(s) from Trello/Github does this address?

What problem does this PR solve?

How did you solve this problem?

How did you make sure your solution works?

Are there any special changes in the code that we should be aware of?

Is there anything else we should know?

Unit tests written?

…2138 #2137

endkeyCoder · 2021-06-29T16:02:43Z

corrections to issues #2137 and #2138

comountainclimber · 2021-06-29T16:06:16Z

app/components/Contacts/ContactsPanel/ContactsPanel.jsx

+    contactChains: [],
+  }
+
+  componentDidMount() {


nitpick: I think something like -

async componentDidMount() { const contactChains = await getChains() this.setState(prevState => ({ ...prevState, contactChains})) }

is a little cleaner

comountainclimber · 2021-06-29T16:07:29Z

app/components/Contacts/ContactsPanel/ContactsPanel.jsx

-      name,
-      address,
-    }),
+  const contactsSpecifcChain = contactChains.filter(


typo here and I think const chainSpecificContacts makes a little more sense

app/actions/contactsActions.js

comountainclimber · 2021-06-29T16:37:04Z

When spinning up this branch the contacts screen crashes the application:

I am also a little confused on the general approach with this solution... Why not just add a new key to storage and follow the exact same logic already in place switching based on chain? something like this:

const STORAGE_KEY = 'addressBook'
const N3_STORAGE_KEY = 'n3AddressBook'

const getContacts = async (chain): Promise<Contacts> => getStorage(chain === 'neo3' ? N3_STORAGE_KEY : STORAGE_KEY)

(And then follow the exact same structure for all the functions/methods)

comountainclimber · 2021-06-29T16:39:20Z

app/components/Contacts/ContactsPanel/ContactsPanel.jsx

@@ -20,6 +20,7 @@ import CopyToClipboard from '../../CopyToClipboard'
 import LogoWithStrikethrough from '../../LogoWithStrikethrough'


I think ideally this file would actually not be updated at all in this PR 👀

endkeyCoder · 2021-06-29T17:07:01Z

When spinning up this branch the contacts screen crashes the application:

I am also a little confused on the general approach with this solution... Why not just add a new key to storage and follow the exact same logic already in place switching based on chain? something like this:
const STORAGE_KEY = 'addressBook'
const N3_STORAGE_KEY = 'n3AddressBook'

const getContacts = async (chain): Promise<Contacts> => getStorage(chain === 'neo3' ? N3_STORAGE_KEY : STORAGE_KEY)
(And then follow the exact same structure for all the functions/methods)

basically if you put conditionals inside the code to validate which chain should use, if at some point there is a need to implement new chains, there will be a need to put new conditionals to validate this, if you persist the chain in storage and use a "filter" function to display your data, no matter how many are added, the behavior of the code remains the same

I'll see what's going on with the contact rendering logic, but I think this is the best approach

comountainclimber · 2021-06-29T17:17:40Z

Ok makes sense! With the new data structure will be relatively simple to keep the same code and support new chains. Lets get that crashing bug sorted out and then we can merge

endkeyCoder · 2021-06-29T17:26:35Z

Ok makes sense! With the new data structure will be relatively simple to keep the same code and support new chains. Lets get that crashing bug sorted out and then we can merge

This is amazing! Let with me, I'll fix it

endkeyCoder · 2021-06-29T21:35:44Z

@comountainclimber everything has been changed according to your suggestions, as soon as you can let me know if you agree or need to make further changes

comountainclimber · 2021-06-30T19:17:37Z

@endkeyCoder its no longer crashing but when I login using an n3 address I am seeing all of the contacts from my neo legacy contact list we need to fix this before merging... To reproduce login with a neo2 address, create a contact and then login with an n3 address

comountainclimber

See comment above about contact list chain issue

endkeyCoder · 2021-06-30T19:37:00Z

its no longer crashing but when I login using an n3 address I am seeing all of the contacts from my neo legacy contact list we need to fix this before merging... To reproduce login with a neo2 address, create a contact and then login with an n3 address

you can see why these contacts were created in a version that didn't separate the chains, if you edit the contact and save it persists that contact in the current configuration, n3 or neo legacy, you want me to do a function that when starting the app put all pre-existing contacts as neo legacy?

comountainclimber · 2021-06-30T19:57:49Z

☝️ You should not be able to see Neo legacy contacts when logging in with an n3 address (and also the opposite should be true)

endkeyCoder · 2021-06-30T20:09:00Z

☝️ You should not be able to see Neo legacy contacts when logging in with an n3 address (and also the opposite should be true)

ok, so should I create a function that check the contacts created in back versions and set this as n3 or neo legacy depending on address?

comountainclimber · 2021-06-30T20:12:55Z

Not sure... there are probably many different ways to solve this but I would do your best to leave the legacy code functioning as it does currently so not to introduce any regressions

endkeyCoder · 2021-06-30T20:20:15Z

Not sure... there are probably many different ways to solve this but I would do your best to leave the legacy code functioning as it does currently so not to introduce any regressions

from what you told me, this should solve it, so the pre-existing contacts will be validated in the same way as the new ones.

I'll do it and you test it and tell me if it's as expected, ok?

comountainclimber · 2021-06-30T21:27:38Z

app/actions/contactsActions.js

+  contactKey: string,
+  chain: string,
+  oldContactKey: string,
+) => {


Where is the validation of n3 addresses here?

it's in chain parameter

comountainclimber · 2021-06-30T21:29:06Z

I think we may be overcomplicating things a little bit but either way n3 addresses need the same validation and flow we are currently applying to neo legacy addresses...

The goal here is to recreate the exact functionality that exists in the neo legacy contacts screen

comountainclimber · 2021-06-30T21:39:35Z

app/actions/contactsActions.js

@@ -5,13 +5,79 @@ import { has, isEmpty, keys, values, indexOf, zipObject, omit } from 'lodash-es'

 import { getStorage, setStorage } from '../core/storage'

+export const ID = 'contacts'
+
+export type TChains = { contactKey: string, chain: string }[]


Dont we need name and address in here too 🤔

don't is necessary, the addressBook storage just is a "auxiliary table"

I like this solution because we avoid redundant data

endkeyCoder · 2021-07-01T13:48:15Z

I think we may be overcomplicating things a little bit but either way n3 addresses need the same validation and flow we are currently applying to neo legacy addresses...

The goal here is to recreate the exact functionality that exists in the neo legacy contacts screen

I understand your thought, but I think that this is a best aprroach to future maintenance

…from n2 in the same way

comountainclimber · 2021-07-01T15:16:27Z

The latest issue I can see:

lllwvlvwlll · 2021-07-01T15:29:41Z

@endkeyCoder this is meant to be a short term implementation so we can ship N3 support quickly. The contacts list behavior will be very different in the next major release. It will align more closely with the mobile wallet approach.

comountainclimber · 2021-07-01T16:06:22Z

👆 definitely... There is actually no need to add any additional code/logic other than the bare minimum needed to support N3 I am worried about making things overly complex here when the changes necessary could be substantially smaller than what this PR is currently doing, however I trust your expertise @endkeyCoder so lets fix the broken typechecking/tests and get this merged

comountainclimber · 2021-07-01T16:08:47Z

For more specific context, I was expecting something along these lines which when used actually requires little/no updates to any JSX files or component data:

// @flow
import { createActions } from 'spunky'
import { wallet } from '@cityofzion/neon-js'
import { wallet as n3Wallet } from '@cityofzion/neon-js-next'
import { has, isEmpty, keys, values, indexOf, zipObject, omit } from 'lodash-es'

import { getStorage, setStorage } from '../core/storage'
import { getSettings } from './settingsActions'

type Contacts = {
  [name: string]: string,
}

const STORAGE_KEY = 'addressBook'
const N3_STORAGE_KEY = 'n3AddressBook'

const getContacts = async (chain: string): Promise<Contacts> =>
  chain === 'neo3' ? getStorage(N3_STORAGE_KEY) : getStorage(STORAGE_KEY)

const setContacts = async (contacts: Contacts, chain: string): Promise<any> =>
  setStorage(chain === 'neo3' ? N3_STORAGE_KEY : STORAGE_KEY, contacts)

const validateContact = (name: string, address: string, chain: string) => {
  if (isEmpty(name)) {
    throw new Error('Name cannot be empty.')
  }
  if (
    chain === 'neo3' ? !n3Wallet.isAddress(address) : !wallet.isAddress(address)
  ) {
    throw new Error(`Invalid address ${address}.`)
  }
}

export const ID = 'contacts'
export const addContactActions = createActions(
  ID,
  ({
    name,
    address,
    chain,
  }: {
    name: string,
    address: string,
    chain: string,
  }) => async (): Promise<Contacts> => {
    validateContact(name, address, chain)

    const contacts = await getContacts(chain)

    if (has(contacts, name)) {
      throw new Error(`Contact "${name}" already exists.`)
    }

    const newContacts = { ...contacts, [name]: address }
    await setContacts(newContacts, chain)

    return newContacts
  },
)

export const updateContactActions = createActions(
  ID,
  ({
    oldName,
    newName,
    newAddress,
    chain,
  }: {
    oldName: string,
    newName: string,
    newAddress: string,
    chain: string,
  }) => async (): Promise<Contacts> => {
    validateContact(newName, newAddress, chain)

    const contacts = await getContacts(chain)
    const names = keys(contacts)
    const addresses = values(contacts)
    const index = indexOf(names, oldName)

    if (index === -1) {
      throw new Error(`Contact "${oldName}" does not exist.`)
    }

    const newContacts = zipObject(
      [...names.slice(0, index), newName, ...names.slice(index + 1)],
      [...addresses.slice(0, index), newAddress, ...addresses.slice(index + 1)],
    )
    await setContacts(newContacts, chain)

    return newContacts
  },
)

export const deleteContactActions = createActions(
  ID,
  ({ name, chain }: { name: string, chain: string }) => async (): Promise<
    Contacts,
  > => {
    const contacts = await getContacts(chain)

    if (!has(contacts, name)) {
      throw new Error(`Contact "${name}" does not exist.`)
    }

    const newContacts = omit(contacts, name)
    await setContacts(newContacts, chain)

    return newContacts
  },
)

export default createActions(ID, () => async (): Promise<Contacts> => {
  const settings = await getSettings()
  const { chain } = settings
  return getContacts(chain)
})

comountainclimber · 2021-07-01T16:10:13Z

app/actions/contactsActions.js

+}
+
+export const getContacts = async (): Promise<Contacts> =>
+  getStorage(STORAGE_KEY)


without chain data this is always going to return the neo legacy contacts 👆

about it function, you prefer to return already filtered by chain contacts, is that it? because in contact screen I make the filter and show only contacts that own a specific chain

We will want the redux state tree to always have the right contacts in place so we need to filter one level higher, I am afraid filtering at the component level will lead to more work and unexpected bugs

ok, I understood, so I gonna the filter in high level

coveralls · 2021-07-01T20:33:03Z

Pull Request Test Coverage Report for Build 7868

10 of 49 (20.41%) changed or added relevant lines in 1 file are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.4%) to 39.436%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
app/actions/contactsActions.js	10	49	20.41%

Files with Coverage Reduction	New Missed Lines	%
app/actions/contactsActions.js	1	13.11%

Totals
Change from base Build 7846:	-0.4%
Covered Lines:	1553
Relevant Lines:	3547

💛 - Coveralls

… list items, so it directs the user to the appropriate link, as per Neo Version related to that record #2139

comountainclimber · 2021-07-02T14:07:41Z

#2140 (comment) this last bug needs to be fixed before we can merge

* Loading activity with defects * fix layout issues * Remove hardcoded GAS in activity tab * add n3 linking for to dora * Fix test * Wire up pending transaction flow * Wires up pending tx flow fixes type errors * Fix possible balance bug

* Remove fee buttons if chain is n3 * Fix test

* Remove fee buttons if chain is n3 * Fix test * Adds support for any nep11 * lint * Fixes pending tx actions

…on-3-II

comountainclimber · 2021-07-20T15:21:15Z

@endkeyCoder @melanke in the future lets always avoid merging broken commits into dev master any other working branches

correction in contacts screen to know when it is neo 3 or neoLegacy #…

57959e8

…2138 #2137

comountainclimber reviewed Jun 29, 2021

View reviewed changes

app/actions/contactsActions.js Outdated Show resolved Hide resolved

comountainclimber reviewed Jun 29, 2021

View reviewed changes

app/actions/contactsActions.js Show resolved Hide resolved

comountainclimber reviewed Jun 29, 2021

View reviewed changes

some corrections in contacts feature

0197353

comountainclimber requested changes Jun 30, 2021

View reviewed changes

comountainclimber reviewed Jun 30, 2021

View reviewed changes

Fixed bug to show only contacts from n3 when configured for this and …

b6f123c

…from n2 in the same way

check if chains is array before make a query

8f126ad

comountainclimber reviewed Jul 1, 2021

View reviewed changes

type errors fix

3215f6a

[Contacts] Implement improvements to Dora activity link available for…

103b7b4

… list items, so it directs the user to the appropriate link, as per Neo Version related to that record #2139

comountainclimber and others added 14 commits July 7, 2021 18:54

feature: support for activity screen (#2142)

72c424d

* Loading activity with defects * fix layout issues * Remove hardcoded GAS in activity tab * add n3 linking for to dora * Fix test * Wire up pending transaction flow * Wires up pending tx flow fixes type errors * Fix possible balance bug

Fixes reset check (#2143)

d3bbfaf

Feature/n3 tx refactor ii (#2144)

7814a64

* Remove fee buttons if chain is n3 * Fix test

feature: additional nep17 support (#2146)

8d414c8

* Remove fee buttons if chain is n3 * Fix test * Adds support for any nep11 * lint * Fixes pending tx actions

Bump neon-js version

931b805

add broadcasting notification

afc2f0f

Bump version in pack

90df8a7

Update snaps

b8b5b34

Fix electron crashing bug

782babb

Fix local storage bug

779606a

bump version in pack

f6a35a6

Update snapshots

718f729

it changed the filter of contacts by chain to action contactActions

ff6bde5

Merge branch 'neon-3-II' of github.com:simplitech/neon-wallet into ne…

9e696b3

…on-3-II

melanke merged commit 09ff8be into CityOfZion:neon-3-II Jul 12, 2021

		@@ -20,6 +20,7 @@ import CopyToClipboard from '../../CopyToClipboard'
		import LogoWithStrikethrough from '../../LogoWithStrikethrough'

correction in contacts screen to know when it is neo 3 or neoLegacy #2140

correction in contacts screen to know when it is neo 3 or neoLegacy #2140

Conversation

endkeyCoder commented Jun 29, 2021

endkeyCoder commented Jun 29, 2021

comountainclimber Jun 29, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comountainclimber commented Jun 29, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

endkeyCoder commented Jun 29, 2021

comountainclimber commented Jun 29, 2021

endkeyCoder commented Jun 29, 2021

endkeyCoder commented Jun 29, 2021

comountainclimber commented Jun 30, 2021 • edited

comountainclimber left a comment

Choose a reason for hiding this comment

endkeyCoder commented Jun 30, 2021 • edited

comountainclimber commented Jun 30, 2021

endkeyCoder commented Jun 30, 2021

comountainclimber commented Jun 30, 2021

endkeyCoder commented Jun 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comountainclimber commented Jun 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

endkeyCoder commented Jul 1, 2021

comountainclimber commented Jul 1, 2021

lllwvlvwlll commented Jul 1, 2021

comountainclimber commented Jul 1, 2021 • edited

comountainclimber commented Jul 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 1, 2021

Pull Request Test Coverage Report for Build 7868

💛 - Coveralls

comountainclimber commented Jul 2, 2021 • edited

comountainclimber commented Jul 20, 2021 • edited

comountainclimber Jun 29, 2021 •

edited

comountainclimber commented Jun 29, 2021 •

edited

comountainclimber commented Jun 30, 2021 •

edited

endkeyCoder commented Jun 30, 2021 •

edited

comountainclimber commented Jul 1, 2021 •

edited

comountainclimber commented Jul 1, 2021 •

edited

comountainclimber commented Jul 2, 2021 •

edited

comountainclimber commented Jul 20, 2021 •

edited