[WIP] Adds wallet properties implementation #293
Conversation
| @@ -238,6 +238,10 @@ void PaymentsServiceImpl::OnWalletCreated(ledger::Result result) { | |||
| TriggerOnWalletCreated(result); | |||
| } | |||
|
|
|||
| void PaymentsServiceImpl::OnWalletProperties(ledger::WalletProperties result) { | |||
There was a problem hiding this comment.
we don't want to expose ledger outside of the payments service. What exactly is in wallet properties?
There was a problem hiding this comment.
this are the fields https://github.com/brave-intl/bat-native-ledger/pull/26/files#diff-2779bd349aa484b3cafcf4c1a138efd5R22. We use them all in UI, like balance, grants, range and choices for monthly contribution
There was a problem hiding this comment.
I think we have individual methods for the balance and we should add them for anything else you need
There was a problem hiding this comment.
but we should never directly expose anything in ledger. For Grants you might want to do something like ContentSite to expose whatever fields we need in the UI
There was a problem hiding this comment.
basically we should only expose the things in payment service that we actually use in the brave rewards ui and leave all the internal details inside of ledger
There was a problem hiding this comment.
if any kind of translation needs to be made to put it in the format we want for the user, that translation should happen in the payment service
NejcZdovc
force-pushed
the
walletProperties
branch
5 times, most recently
from
a2b26a5 to
93954ee
Compare
browser/ui/webui/brave_rewards_ui.cc
Outdated
| } | ||
| } | ||
| } | ||
| web_ui()->CallJavascriptFunctionUnsafe("brave_rewards.walletProperties"); |
There was a problem hiding this comment.
wouldn't this be a generic method to update the state with the new webui props?
There was a problem hiding this comment.
yeah I still need to check if generic could be applied here (because I need to check all cases). Problem is that we could be doing some specific actions after we update state and we would need to know which action was triggered to for example reset some timers or something like this
There was a problem hiding this comment.
we can also keep the balance update timers in c++ and just update the props
There was a problem hiding this comment.
we talked about this with @bbondy and we decided to have all timers in app and only do calls to library when needed
There was a problem hiding this comment.
well, I'm not even a big fan of the fact that the balance update runs continuously when the payments ui is active and we definitely don't want it running all the time. I think it probably makes sense to put that inside the webcontents observer though and not JS
There was a problem hiding this comment.
balance is running only when you are on rewards page (chrome://rewards). We are already quite smart about it, so for example when you claim grant we call balance etc, so maybe we can have this conversation again and increase timer from a minute to something higher
| case types.GET_WALLET_PROPERTIES: | ||
| chrome.send('getWalletProperties', []) | ||
| break | ||
| case types.ON_WALLET_PROPERTIES: |
There was a problem hiding this comment.
I see, so you're treating the response as an action to update the store? That seems like a reasonable route to go as well. I was thinking thinking something more like writing webui properties as store.propertyname and using a generic mechanism to write those to the actual store for the page, but either way works.
There was a problem hiding this comment.
so to basically have 1:1 mapping with redux store? this is how I did so far https://github.com/brave/brave-core/pull/293/files#diff-aa3505dbf36b5d03d8ba0751e0c99904R124 which I think it's quite the same as you were thinking.
NejcZdovc
force-pushed
the
walletProperties
branch
2 times, most recently
from
ad6e622 to
1514e65
Compare
NejcZdovc
force-pushed
the
walletProperties
branch
from
1514e65 to
ac070ac
Compare
|
closing in favour of #241 |
Submitter Checklist:
git rebase -ito squash commits (if needed).Test Plan:
Reviewer Checklist: