Skip to content

Conversation

Serhii-Borodenko
Copy link
Contributor

@Serhii-Borodenko Serhii-Borodenko commented Jun 13, 2025

Issue Number (if Applicable): Fixes #

Description

Please include a summary of the changes and which issue is fixed / feature is added.

Pull Request - Checklist

  • Initial Manual Tests Passed
  • Double check modified code and verify it with the feature/task requirements
  • Format code
  • Look for code duplication
  • Clear naming for variables and methods
  • Manual tests in accessibility mode (TalkBack on Android) passed

Copy link
Contributor

@OmarHatem28 OmarHatem28 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still have a lot of files to review, but since we want to get done with this, I will submit my review so far, and please review the code once more after your last changes and let me know

label: 'X',
iconPath: 'assets/images/x_social.png',
alias: '@username',
supportedCurrencies: [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should support CryptoCurrency.all

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can’t use it safely because it may cause wrong validation issues — for example, Polygon could be detected as Ethereum and Doge as PIVX. I’ll reuse AddressValidator.reliableValidateCurrencies here for all cases like ‘X’ (searching inside the string). I use this list on the welcome page as well for the same reason.

default:
return (await ens.withName(name).getAddress()).hex;
}
if (coinType == CoinType.ETH) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about polygon? i.e || CoinType.Matic

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you missed this comment

onSelectedContact?.call((contact.name, contact.address));
}

if (contact is (ContactRecord,String)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are you returning a pair, why the address is not included in the ContactRecord class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ContactRecord doesn’t contain an address anymore — addresses are stored in parsedBlocks and manual. Here I just need to return the contact name and the address the user picked. I can either return a tuple, or return a separate map with the name and picked address

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't get what exactly needed to be fixed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we hiding the add to contact bottom sheet after a successful send?

default:
return (await ens.withName(name).getAddress()).hex;
}
if (coinType == CoinType.ETH) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you missed this comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we hiding the add to contact bottom sheet after a successful send?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants