Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable Reclaiming of Escrowed Payments #10

Merged
merged 20 commits into from
Aug 2, 2019
Merged

Enable Reclaiming of Escrowed Payments #10

merged 20 commits into from
Aug 2, 2019

Conversation

medhakothari
Copy link
Contributor

@medhakothari medhakothari commented Jul 18, 2019 •
edited by sallyjyl
Loading

Description

Allows for the reclaiming of escrowed payments. Once the payment has expired, it gives the sender a notification on the wallet to either remind the sender through a text message, or allows the sender to reclaim the payment.

Tested

Tested the following tests between two accounts.

  • Alice sends an escrowed payment to Bob. Payment hasn't expired yet, so notification does not show up to Alice
  • Alice sends an escrowed payment to Bob. Payment expires, and Alice gets a notification on the home screen of wallet app. Alice is able to send a message or reclaim the payment successfully.
  • Alice sends an escrowed payment to Bob. Bob withdraws the payment. The payment expires, but as it was already withdrawn, the notification does not show up to Alice.
  • Alice sends an escrowed payment to Bob. The payment expires, and Alice reclaims the payment. Bob attempts to withdraw the payment, and is able to successfully redeem the invite, but not the escrowed payment

Screenshots of the notification and the Reclaim Payment Confirmation Screen:
Screen Shot 2019-07-25 at 3 07 03 PM Screen Shot 2019-07-25 at 3 07 53 PM

Other changes

  • I added additional testing for the escrow smart contract

Related issues

Backwards compatibility

none

@medhakothari medhakothari requested review from jmrossy and removed request for jmrossy July 19, 2019 00:58
jmrossy
jmrossy reviewed Jul 22, 2019
View reviewed changes
Copy link
Contributor

@jmrossy jmrossy left a comment

Choose a reason for hiding this comment

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

Good progress! Some comment below :)

packages/mobile/src/escrow/actions.ts Outdated Show resolved Hide resolved
packages/mobile/src/escrow/actions.ts Outdated Show resolved Hide resolved
packages/mobile/src/escrow/actions.ts Outdated Show resolved Hide resolved
packages/mobile/src/escrow/actions.ts Outdated Show resolved Hide resolved
packages/mobile/src/notifications/EscrowedPaymentReminderNotification.tsx Outdated Show resolved Hide resolved
packages/mobile/src/escrow/saga.ts Outdated Show resolved Hide resolved
packages/mobile/test/values.ts Outdated Show resolved Hide resolved
@jmrossy jmrossy assigned medhakothari and unassigned jmrossy Jul 22, 2019
@medhakothari medhakothari assigned jmrossy and unassigned medhakothari Jul 23, 2019
jmrossy
jmrossy reviewed Jul 23, 2019
View reviewed changes
Copy link
Contributor

@jmrossy jmrossy left a comment

Choose a reason for hiding this comment

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

Two small things, otherwise LGTM

packages/mobile/src/escrow/reducer.ts Outdated Show resolved Hide resolved
packages/mobile/src/escrow/saga.ts Outdated Show resolved Hide resolved
@jmrossy jmrossy assigned medhakothari and unassigned jmrossy and cmcewen Jul 23, 2019
@jmrossy jmrossy assigned medhakothari and unassigned jmrossy Jul 24, 2019
@medhakothari medhakothari assigned jmrossy and unassigned medhakothari Jul 24, 2019
jmrossy
jmrossy reviewed Jul 25, 2019
View reviewed changes
Copy link
Contributor

@jmrossy jmrossy left a comment

Choose a reason for hiding this comment

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

Almost there! Just some nits, we should be able to merge today.
Also, please include a screenshot here.

packages/mobile/src/escrow/ReclaimPaymentConfirmationScreen.tsx Outdated
const payment = this.getReclaimPaymentInput()
const convertedAmount = divideByWei(payment.amount.toString())
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a toString() here?
It'd be more clever to make divideByWei accept other types, including null values :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's bc payment.amount is a BigNumber not bc its null
divideByWei can't take BigNums

Now that I think if it, BigNumber.valueOf() might be better and more explicit than toString(). Will change it to that

packages/mobile/src/escrow/ReclaimPaymentConfirmationScreen.tsx Outdated Show resolved Hide resolved
packages/mobile/src/escrow/ReclaimPaymentConfirmationScreen.tsx Outdated Show resolved Hide resolved
@jeanregisser jeanregisser mentioned this pull request Aug 1, 2019
Merged
@jmrossy jmrossy merged commit 1d17771 into master Aug 2, 2019
@jmrossy jmrossy deleted the medhak1/enableReclaim branch August 2, 2019 10:24
H34D referenced this pull request in H34D/celo-monorepo Nov 26, 2019
@aaitor
Merge pull request #10 from celo-org/master
eb20309 
master
This was referenced Jul 5, 2020
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmrossy jmrossy jmrossy approved these changes

Assignees

@jmrossy jmrossy

Labels
wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

User SBAT reclaim funds associated with an unused invite after a certain period
3 participants
@medhakothari @jmrossy @cmcewen