Komenci <> Valora Integration: invites and escrow #5742
Conversation
…d route accordingly
…o-org/celo-monorepo into tarikbellamine/komenci-valora-escrow
| // Needed to ensure that codes inputted in quick succession are not | ||
| // assigned the same nonce by Komenci | ||
| while (yield select(feelessProcessingInputCodeSelector)) { | ||
| yield delay(100) |
There was a problem hiding this comment.
Is the delay actually important or are we just interested in reacting to redux state changes?
in that case we could replace the delay by yield take() which would wait for any dispatched action.
There was a problem hiding this comment.
It may not be as simple as yield take given we want them to be taken one-by-one and there may be 2 codes "waiting". I've left a TODO to look into this later.
| }, []) | ||
|
|
||
| useEffect(() => { | ||
| if (status.isVerified) { | ||
| dispatch(setNumberVerified(true)) | ||
| } | ||
| }, [verificationState.status.isVerified, feelessVerificationState.status.isVerified]) |
There was a problem hiding this comment.
| }, [verificationState.status.isVerified, feelessVerificationState.status.isVerified]) | |
| }, [status.isVerified]) |
There was a problem hiding this comment.
I think we want to leave this as is. There is an edge case where a user is allowed to tryFeeless but they've already verified in the non-feeless flow.
There was a problem hiding this comment.
I understand we might not want to change this at the last minute but the dependencies listed should only be values that are used in the useEffect block.
| @@ -124,7 +130,7 @@ export function* feelessFetchVerificationState() { | |||
| ]) | |||
|
|
|||
| const komenciKit = new KomenciKit(contractKit, walletAddress, { | |||
| url: KOMENCI_URL, | |||
| url: feelessVerificationState.komenci.callbackUrl || KOMENCI_URL, | |||
There was a problem hiding this comment.
Do we still need the fallback?
There was a problem hiding this comment.
Yes, this has to do with data center routing.
| @@ -92,7 +98,7 @@ import { TransactionReceipt } from 'web3-eth' | |||
|
|
|||
| const TAG = 'identity/feelessVerification' | |||
|
|
|||
| const KOMENCI_URL = 'https://komenci.celo-networks-dev.org' | |||
| export const KOMENCI_URL = 'https://staging-komenci.azurefd.net' | |||
There was a problem hiding this comment.
This is only temporary right?
There was a problem hiding this comment.
The placement was temporary, the url is not. That will be our staging URL moving forward.
| useEffect(() => { | ||
| if (reviewButtonPressed) { | ||
| if (recipientVerificationStatus === RecipientVerificationStatus.UNKNOWN) { | ||
| // Wait until the recipient status is fetched. | ||
| return | ||
| } else if ( | ||
| recipientVerificationStatus === RecipientVerificationStatus.UNVERIFIED && | ||
| dollarAmount.isGreaterThan(MAX_ESCROW_VALUE) | ||
| ) { | ||
| const maxAmount = convertDollarsToLocalAmount(MAX_ESCROW_VALUE, localCurrencyExchangeRate) | ||
| dispatch( | ||
| showError(ErrorMessages.MAX_ESCROW_TRANSFER_EXCEEDED, ALERT_BANNER_DURATION, { | ||
| maxAmount: maxAmount?.toFixed(2), | ||
| symbol: localCurrencySymbol, | ||
| }) | ||
| ) | ||
| } else { | ||
| isOutgoingPaymentRequest ? onRequest() : onSend() | ||
| } | ||
| setReviewButtonPressed(false) | ||
| } | ||
| }, [reviewButtonPressed, recipientVerificationStatus]) |
There was a problem hiding this comment.
There's a slight bug here since localCurrencyExchangeRate and localCurrencySymbol are not the in dependencies list of useEffect. Though in practice those values won't change at this point.
Adding them to the dependencies list is not enough though, we'd also need to make sure the action is not dispatched twice. Which would happen if other values change.
There was a problem hiding this comment.
True, but I feel pretty comfortable leaving it as is. Nothing bad happens if the user is over the limit here; it's an artificial constraint we are placing as an added security measure. There is also no way to change localCurrency on the screen.
Is there a use case you had in mind where this could be problematic?
There was a problem hiding this comment.
Again it's harmless, localCurrency doesn't change but localCurrencyExchangeRate can change.
The point is that it's not entirely right and introduces a subtle bug. Which might or might not bite us in the future ;)
….com/celo-org/celo-monorepo into tarikbellamine/komenci-valora-escrow
….com/celo-org/celo-monorepo into tarikbellamine/komenci-valora-escrow
|
Hi @tarikbellamine Verified task on latest test flight build v1.5.5 (34) and Android play store internal build v1.5.5 (1004294323) and observe the followings
Can you please provide more information about this task. |
Description
This PR builds on the previous two PRs of the series (DEK/wallet registration and feeless verification to provide an escrow experience that does not use "invite codes".
This PR includes unmerged changes included in previous PRs. The most relevant files for this PR:
The pertinent UI changes:
The transaction changes:
paymentIdsand the private key required to redeeem are now both formed using a hash of the recipient's pepperOther changes
N/A
Tested
Not yet
Related issues
Backwards compatibility
No