-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat: connect faucets component #139
Conversation
lucachaco
commented
Feb 2, 2022
•
edited
Loading
edited
- Moving most of the logic into the faucet component
- RBTC balance added
src/screens/home/index.tsx
Outdated
setRifToken( | ||
balances.find(token => { | ||
return token.symbol === 'tRIF' | ||
}), | ||
) | ||
setRbtcToken( | ||
balances.find(token => { | ||
return token.symbol === 'TRBTC' | ||
}), | ||
) |
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.
Your code works well, but for better readability, it could be written like this:
setRifToken(balances.find(token => token.symbol === 'tRIF'))
setRbtcToken(balances.find(token => token.symbol === 'TRBTC'))
Since you are only returning a single line, you can omit the {
and return
.
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.
Code is fine, however since the RBTC balance isn't loading it is hard to fully test.
One quick comment about code style.
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.
Should we add the RBTC balance to the global state in some way? We can add this together with the ticket to show back the RBTC in the portfolio
Hmm it is in the global state now with this and it shown in the portfolio |
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.
Just one quick change on the rBTC balance and then some cleanup.
src/screens/home/index.tsx
Outdated
|
||
const [selected, setSelected] = useState<ITokenWithBalance | null>(null) | ||
|
||
const [selectedPanel, setSelectedPanel] = useState<string>('portfolio') | ||
|
||
const loadRBTCBalance = async () => { | ||
const rbtcBalanceEntry = await wallet.provider!.getBalance( | ||
wallet.smartWallet.address, |
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 think we need to get the RBTC balance of the EOA wallet not the smartwallet.
src/screens/home/index.tsx
Outdated
if (!selected) { | ||
setSelected(balances[0]) | ||
} | ||
}, [balances]) | ||
|
||
loadRBTCBalance().then(() => console.log('RTBC loaded')) |
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.
Remove the console.log please.
src/screens/home/index.tsx
Outdated
|
||
const selectedTokenColor = getTokenColor(selected?.symbol) | ||
|
||
const containerStyles = { | ||
shadowColor: setOpacity(selectedTokenColor, 0.5), | ||
} | ||
// |
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.
remove this please.
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.
Sorry, the comment before was mistaken. I didn't see it added RBTC back. Great job!
We then need to work on some refactor. Home screen should not be responsible of subscribing to RBTC balance, we should work that in Core
src/screens/home/index.tsx
Outdated
}, [balances]) | ||
|
||
loadRBTCBalance().then(() => console.log('RTBC loaded')) | ||
}, [state.balances]) |
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.
src/screens/home/index.tsx
Outdated
|
||
const [selected, setSelected] = useState<ITokenWithBalance | null>(null) | ||
|
||
const [selectedPanel, setSelectedPanel] = useState<string>('portfolio') | ||
|
||
const loadRBTCBalance = async () => { | ||
const rbtcBalanceEntry = await wallet.provider!.getBalance( |
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.
Open discussion, should we do this in the backend?
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.
That might be a good idea since that way the service could easily be used for other apps, such as a browser wallet, or perhaps the rBench or Identity Manager. With one call you can get everything from the service.
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.
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.
LGTM
src/screens/home/index.tsx
Outdated
useEffect(() => { | ||
const balances = Object.values(state.balances) | ||
|
||
if (!selected) { | ||
setSelected(balances[0]) | ||
} | ||
}, [balances]) | ||
}, [state.balances]) |
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.
What is this used for?
src/screens/home/index.tsx
Outdated
useEffect(() => { | ||
setInterval(async () => { | ||
console.log('Loading RBTC') | ||
loadRBTCBalance().then() | ||
}, 5000) | ||
}, []) |
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 would move this to another part of the code. Maybe in Core or in subscriptions.
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 tried moving the effect, is working fine. Still the RBTC faucet appears for a while before the RBTC balance has loaded.
For a new wallet:
Screen.Recording.2022-02-08.at.11.33.28.mov
For a wallet that has some other balances:
Screen.Recording.2022-02-08.at.12.11.16.mov
We can keep it this way now but we need to find a better solutions for loading states
src/subscriptions/RIFSockets.tsx
Outdated
if (isWalletDeployed || isDeployed) { | ||
const connect = async () => { | ||
rifServiceSocket?.on('init', result => { | ||
interval = setInterval(async () => { |
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 could start even if rifServiceSocket
didn't init. I would do it in a different effect
React.useEffect(() => {
const interval = setInterval(async () => {
loadRBTCBalance(wallet, dispatch)
}, 5000)
return () => clearInterval(interval)
}, [wallet])
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.
LGTM! Fixing the thing of the balance decimals. The balancetoString
is working wrong
* Section component to render options in settings * Implementing sections * Adding translate to section titles * Adding props to setting screen * Passing deleteKeys function down to the settings screen * removing delete keys button * Adding condition in case the key doesnt exist in the send screen and also deleting the deletekeys button from keysinfo screen * Fixing test * Allow the user to add new accounts and switch to them (#144) * Add screen for managing current wallet. * Pass addNewWallet to manage wallets screen. * Allow the user to switch the selectedWallet in the core. * Show addresses in manage screen. * In sockets file, move connect() feature outside of the useEffect. * combine functions into single useEffect and remove check for if wallet is deployed. * Remove unused variables, add sockets method and use it. * Add wallet and isDeployed to the list when clicking the button. * Clean up addWallet code. * Move next wallet creation to the operations file. * Pass networkId * Skipping these tests, see PR description. * Export props and use in RootNavigation * Enhancemnt/no usd value (#146) * Adding not available message to the balance row component * Linting * Simplifying quota prop * Update src/screens/home/BalanceRowComponent.tsx * Make USD text smaller Co-authored-by: Agustin Villalobos <agustin.villalobos@iovlabs.org> Co-authored-by: Ilan <36084092+ilanolkies@users.noreply.github.com> Co-authored-by: Ilan <ilanolkies@outlook.com> * feat: connect faucets component (#139) * feat: connect faucets component * chore: add comment * feat: add rbtc balance * feat: add rbtc balance * fix: refactor test * chore: remove console * fix: validate balances * format: lint code * fix: load rbtc balance with serInterval * fix: move rbtc load code * format: lint code * fix: move code * fix: remove duplicated line * fix: add missing types * fix: add todo comment * fix: move timer * fix: add different effect * Fix balanceToString Co-authored-by: Ilan <ilanolkies@outlook.com> * Enhancement/live txs (#129) * added live balances * minor changes * RIF Sockets Context * URL based on platform and exporting rifWalletServicesUrl variable * Adding socket.io-client * Wrapping app with RIFSocketsProvider * Implementing live prices * Lifting state for activities/transactions * Lifting balances state * Lint * Update src/core/setup.ts Co-authored-by: Christian Escalante <chescalante.ar@gmail.com> * Lint * Fixing test * Move service url to config * connected home with tokens and transactions state * fixed tests and added polimophic socket class (#121) Co-authored-by: Christian Escalante <chescalante@gmail.com> * Moved subscriptions instance to Core + added ABI Enhancer after receiving a transaction * Lint * Changing to new state structure * Changing state structure to match enhanced transactions * Implementing top 5 enhanced transactions * Implementing FlatList component to render transactionsç * Implementing FlatList to implement infinite scrolling * Lint * Fixing test * Removing data array from the state structure, because its duplicating the txs * Lint * Sorting by timestamp * Fixing sorting funciton * Changing filtering to use hash since its unique Co-authored-by: Christian Escalante <chescalante@gmail.com> Co-authored-by: Agustin Villalobos <agustin.villalobos@iovlabs.org> Co-authored-by: Christian Escalante <chescalante.ar@gmail.com> Co-authored-by: Ilan <ilanolkies@outlook.com> Co-authored-by: Ilan <36084092+ilanolkies@users.noreply.github.com> * Update Active Row Component when balance/wallet changes (#148) * Start working on storing a reference (contractAddress) rather than the object * Clean up comments and use new value. * Remove conosle.log * Remove unnecessary filter and call address directly if available * Section component to render options in settings * Implementing sections * Adding translate to section titles * Adding props to setting screen * Passing deleteKeys function down to the settings screen * removing delete keys button * Adding condition in case the key doesnt exist in the send screen and also deleting the deletekeys button from keysinfo screen * Fixing test * Implemenenting information section * Rebasing Core * Adding testing * Add button to manage wallets. Co-authored-by: Agustin Villalobos <agustin.villalobos@iovlabs.org> Co-authored-by: Jesse Clark <hello@developerjesse.com> Co-authored-by: Ilan <36084092+ilanolkies@users.noreply.github.com> Co-authored-by: Ilan <ilanolkies@outlook.com> Co-authored-by: Luis Chavarría <luis.chavarria@iovlabs.org> Co-authored-by: Christian Escalante <chescalante@gmail.com> Co-authored-by: Christian Escalante <chescalante.ar@gmail.com>