Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
Query and record exchange rate using notification service #1020
Query and record exchange rate using notification service #1020
Changes from 21 commits
ec0fd6c
091e92e
b02a819
86bcbd0
c18fd14
c21bf0d
44256d4
3923663
6400495
d641579
a5bb2f2
4c6d22c
d757f91
1c45663
8bf979a
9ef79fd
65fdb91
ddedead
e7d519f
80a339c
c8eb924
93eea21
15c1df0
70ba895
a9c62fc
4bddc02
2ae3fb0
ce4bedc
e4eebc7
211c973
702df1b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we shouldn't add a dependency on walletkit here given that we want to deprecate that in favor of contract kit anyway. Can you change this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to be consistent with the mobile app, which uses
walletkit
: https://github.com/celo-org/celo-monorepo/blob/master/packages/mobile/src/exchange/actions.ts#L6contractkit
doesn't have the samegetExchangeRate()
function, so usingcontractkit
would mean pasting over the code inwalletkit
.I created an issue to migrate this and the mobile app away from
walletkit
after the exchange (or another util where this and the mobile app can share logic) is implemented incontractkit
. #1026There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated ContractKit with the needed functionality
#1083
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this ip seems likely to change as integration churns right? I don't feel strongly but I'd prefer to have it just error if it can find a web3 provider value. Or, even better, have it skip exchange rate polling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Updated to skip exchange polling if it can't find a web3 provider value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm slightly concerned about the impact of having two things polling at the same time given that JS is single threaded, though admittedly this one doesn't run often. If you can confirm that it doesn't seem to interfere with the notification polling then I think we're good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From running locally, this doesn't seem to interfere, but I'm going to test deploying this to integration to confirm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, any reason to get web3 here given that it's in the same file as handleExchangeQuery?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you mocking web3 for this? if not, I recommend we do.
If you need a semi-realistic web3 mock, you can use the one in
mobile/__mocks__
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After moving the getExchangeRate function to
ContractKit
, these tests don't improve coverage, as they are directly testing a function inContractKit
. So I removed this