Transfer/ error when attempting to transfer to blacklisted address - #2622
Conversation
| // The list may hold addresses in any casing, while the checked address can come straight from | ||
| // user input (typed, pasted or resolved from a name). Compare every common form, so that a | ||
| // lowercase input is never treated as safe only because the list holds it checksummed. | ||
| if (this.#addresses.has(address) || this.#addresses.has(address.toLowerCase())) | ||
| return 'BLACKLISTED' | ||
|
|
||
| try { | ||
| if (this.#addresses.has(getAddress(address))) return 'BLACKLISTED' |
There was a problem hiding this comment.
Are you sure? The addresses returned by the api seem all lowercase. Can you do a quick check with a script and if that's true, lowercase them on add() to ensure that they will be so in the future, add a comment and only check the lowercase version here. I know it's not from this PR, but it's not a good practice to trust the API when storing data and not updating it often.
There was a problem hiding this comment.
the backend currently returns only lower case addresses, but it was not forced by the backend, this is simply how addresses happen to be stored in the upstream github repos. I will update the phishing controller to parse addresses to be lower case always and remove the extra logic here
| this.#addresses = new Set(phishing.addresses) | ||
| // Normalized to lowercase so getAddressBlacklistedStatus can do a plain lookup, regardless of | ||
| // the casing the relayer (or, for pre-normalization storage, an older app version) used. | ||
| this.#addresses = new Set(phishing.addresses.map((address: string) => address.toLowerCase())) |
There was a problem hiding this comment.
There are 4599 addresses which isn't a lot but this will be calculated every time that the mobile app is open. I know it should be rather lightweight, but it's still best practice to do this as a migration instead. Wdyt, is it worth it to write a migration?
PetromirDev
left a comment
PetromirDev
left a comment
There was a problem hiding this comment.
Remove the new init() map and lfg
2 participants
Resolves: https://github.com/AmbireTech/ambire-app/issues/7716
To test
Get some address from the backend and try to transfer funds to it https://cena.ambire.com/api/v3/scamchecker/data