-
Notifications
You must be signed in to change notification settings - Fork 14
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: user hub updated with streams #3018
base: feat/streaming-payments-ui
Are you sure you want to change the base?
feat: user hub updated with streams #3018
Conversation
02346fd
to
8b37a8d
Compare
0c19e06
to
af8a4f5
Compare
0f90696
to
4256eb6
Compare
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.
Nice work on this so far! I've left a few comments.
The UI looks good and I can see the number ticking up here:
Screen.Recording.2024-09-04.at.09.53.46.mov
This is probably a personal preference thing, but the numbers all jumping into place when opening the balance tab is a bit distracting and might be better to just be static when it loads and only animate on update:
Screen.Recording.2024-09-04.at.09.55.02.mov
Reputation tab also looks good:
On mobile I wasn't sure if these buttons should be stacked or not, it seems like the figma mobile design might need updating as there are a few differences.
One thing I wanted to check as it isn't clear from this PR or the original issue - is the 'total claimed' and 'available to claim' supposed to be for specifically within that colony? The "Your overview in Wayne Enterprises" to me suggests it should be for just this colony.
Currently it shows the same value in any colony and is fetching streaming payments for all colonies I believe - see the comment I left regarding this.
I am also experiencing the permissions issue with claiming payments which makes this hard to test fully.
...nts/common/Extensions/UserHub/partials/BalanceTab/partials/StreamsInfoRow/StreamsInfoRow.tsx
Show resolved
Hide resolved
return null; | ||
} | ||
|
||
// @TODO: handle empty state <EmptyContent /> |
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.
Is this TODO out of scope for this PR?
...nts/common/Extensions/UserHub/partials/BalanceTab/partials/BalanceInfoRow/BalanceInfoRow.tsx
Outdated
Show resolved
Hide resolved
const { totalAvailable, totalClaimed } = await calculateTotalsFromStreams( | ||
{ | ||
streamingPayments: items, | ||
currentTimestamp: Math.floor(blockTime ?? Date.now() / 1000), | ||
currency, | ||
colony, | ||
}, | ||
); |
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 not sure this works as it assumes all streaming payments are from the current colony. It seems like the useGetStreamingPaymentsQuery
gets all the streaming payments for that user from every colony, so we should instead group the streaming payments by colony before passing them to this function - or something like that, there may be a better way to do it.
4256eb6
to
8f3cbe2
Compare
@iamsamgibbs thanks for your review! |
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.
Thanks for your PR @joanna-pagepro, I left some code comments for you to look at.
I was not able to test the UI using action form as it's still affected by the bug raised in #2938 regarding wrong units the start/end time is set in, but using the testing UI I can confirm the amounts look correct, and they update every second:
Screen.Recording.2024-09-17.at.11.00.45.mov
listStreamingPayments( | ||
filter: { | ||
recipientAddress: { eq: $recipientAddress } | ||
id: { beginsWith: $colonyId } | ||
} | ||
) { | ||
items { | ||
...StreamingPayment | ||
} | ||
} |
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.
- Can you rebase and change this query to
getStreamingPaymentsByColony
? - The query needs to be paginated since it's likely a colony will have many streams, please have a look at how
nextToken
pagination is implemented elsewhere
src/utils/streamingPayments.ts
Outdated
} | ||
}; | ||
|
||
export const getStatus = ({ |
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.
export const getStatus = ({ | |
export const getStreamingPaymentStatus = ({ |
} | ||
|
||
const balanceInWeiToEth = new Decimal(amount).div( | ||
10 ** getTokenDecimalsWithFallback(decimals, nativeToken?.decimals), |
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.
native token decimals shouldn't be used as fallback here
src/utils/streamingPayments.ts
Outdated
return startTimeValue.lt(currentTimestamp); | ||
}; | ||
|
||
export const checkIfEnded = ({ |
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.
export const checkIfEnded = ({ | |
export const checkIfStreamingPaymentEnded = ({ |
src/utils/streamingPayments.ts
Outdated
}; | ||
}; | ||
|
||
export const checkIfStarted = ({ |
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.
Please keep the names streaming payment specific as it's hard to follow if outside this file
src/types/streamingPayments.ts
Outdated
Active = 'Active', | ||
Cancelled = 'Cancelled', | ||
NotStarted = 'Not started', | ||
Ended = 'Ended', | ||
LimitReached = 'Limit reached', |
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.
Can you make the values match the keys? E.g. NotStarted = 'NotStarted'
. This is so that people don't accidentally use it as labels instead of i18n messages.
await calculateTotalsFromStreams({ | ||
streamingPayments: items, | ||
currentTimestamp: Math.floor(blockTime ?? Date.now() / 1000), | ||
currency, | ||
colony, | ||
}); |
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.
Feels like this could be a hook that manages its own state and data fetching, instead of the component having to call it inside useEffect and then put the results into state.
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 moved this code to a separate hooks.ts
file for better readability.
Can you explain more? I'm not sure what you'd like me to do here.
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's exactly what I meant, thank you
|
||
const getTotalFunds = useCallback( | ||
async (items: StreamingPaymentItems) => { | ||
const { totalAvailable, totalClaimed, isActive } = |
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 isActive
lacks context, it's not clear what it refers to based on the method.
const AvailableToClaimCounter: FC<AvailableToClaimCounterProps> = ({ | ||
amountAvailableToClaim, | ||
getTotalFunds, | ||
isActive, |
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.
as below, can it specify what is active? (at least one stream presumably?)
8bcdc01
to
56c18b2
Compare
@jakubcolony code updated. Please also check my question regarding the hook that you've suggested. |
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.
Thank you @joanna-pagepro for your work and your patience. Nice work, I have noticed a couple of things.
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-wise all looks good, nice work. I can confirm I'm getting the same issues Arren reported, I was only able to claim by manually assigning permissions.
@jakubcolony I think that there's a problem when installing the extension because the enable action is not called if there are no |
Ah, seems like this is likely related to #3027 (I'm assuming it made it's way onto a parent branch somewhere further up the chain) - looks like I mistakenly assumed having no |
56c18b2
to
1b74096
Compare
@arrenv can you check 1. and 2. again? |
Actually on further inspection, the changes in #3027 don't appear to be on this branch, so won't be the cause of any issues here. Apologies for any confusion! (And on even further inspection, #3027 will make the |
Description
User hub updates that are related to the streams
Unfortunately, I couldn't test the
claim
functionality because there was an error stating that some permissions were missing.Testing
Diffs
New stuff ✨
Changes 🏗
Deletions ⚰️
Balance.tsx
because there's a new structure in user hub nowTODO
Contributes to #3016