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.

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

tBTC stats on overview page #490

Merged
merged 16 commits into from
Apr 19, 2023
Merged

Conversation

r-czajkowski
Copy link
Collaborator

Closes: #428
Closes: #458

This PR adds the tBTC stats to the overview page and tBTC bridge page as an empty state before the user connects with a wallet.

Screenshots

obraz

obraz

Extract a common list item component with outline variant as we use them
in several places in dapp. We can't add a new variant using chakra
theming because the `ListItem` is a part of the `List` component and
does not have separate theme. In that case we should pass the eg
`variant="otuline"` to the `List` component but in my opinion does not
fit here because list can be order or unordered.
Take into account the tBTC v2 token total supply. Also here we fix unit
test for this hook.
And display data in component.
To use some tBTC stats in other places in T dapp.
Display tBTC stats as an empty state if user is not connected with a
wallet.
@pdyraga
Copy link
Member

pdyraga commented Apr 6, 2023

Sweet! 🚀

@github-actions
Copy link

github-actions bot commented Apr 6, 2023

We want to redirect users to transaction details in the blockexplorer.
@github-actions
Copy link

github-actions bot commented Apr 6, 2023

Copy link
Contributor

@michalsmiarowski michalsmiarowski left a comment

Choose a reason for hiding this comment

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

First part of the review

...restProps
}) => <RecentDeposits deposits={deposits} {...restProps} />

export const DefaultProtocolHistory: FC<ProtocolHistoryProps> = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking:

Why DefaultProtocolHistory and not just ProtocolHistory?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Comment on lines +73 to +76
[Token.TBTC]: tbtcv1Context,
[Token.T]: tContext,
[Token.Nu]: nuContext,
[Token.TBTCV2]: tBTCContext,
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking:

I think we should decide which naming convention we want to use for both tbtc tokens and stick to use. We can either use [tBTC and tBTCv2] or [tBTCv1 and tBTC]. Messing them up here and there might be problematic later.

We don't have to do it in this PR though. It's just something that will have to be resolved sooner or later and the code needs to be checked for those occurences.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We discussed this here #300 as well and #300 covers this. We need to revisit this PR and finally merge.

)
// The `toUsdBalance` function was called 2x times because it was called

// The `toUsdBalance` and `spyOnFormatUnits` function was called 2x times because it was called
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// The `toUsdBalance` and `spyOnFormatUnits` function was called 2x times because it was called
// The `toUsdBalance` and `spyOnFormatUnits` function were called 2x times because they were called

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

}
}

const fetchRecenttBTCDeposits = async (numberOfDeposits = 4) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking:

I believe we should write TBTC (started with capital T) when it's in the middle of the word that is writtne in camelCase convention. Everywhere else we should stick to tBTC.

Suggested change
const fetchRecenttBTCDeposits = async (numberOfDeposits = 4) => {
const fetchRecentTBTCDeposits = async (numberOfDeposits = 4) => {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

"https://api.thegraph.com/subgraphs/name/suntzu93/threshold-tbtc"

type TransactionRawResponse = {
// Actually transacion hash where the tbtc tokens were minted.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Actually transacion hash where the tbtc tokens were minted.
// Actual transacion hash where the tbtc tokens were minted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

type TransactionRawResponse = {
// Actually transacion hash where the tbtc tokens were minted.
id: string
timestamp: string
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking:

We could add in the comment that this timestamp is in seconds/miliseconds.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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


type TBTCBrdigeStatsProps = ProtocolHistoryProps & TVLProps

export const TBTCBrdigeStats: FC<TBTCBrdigeStatsProps> = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export const TBTCBrdigeStats: FC<TBTCBrdigeStatsProps> = ({
export const TBTCBridgeStats: FC<TBTCBridgeStatsProps> = ({

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@michalsmiarowski
Copy link
Contributor

Also, let's resolve conclifts 📄 ✍️

Comment on lines +29 to +33
jest.mock("../../web3/hooks/useTBTCv2TokenContract", () => ({
...(jest.requireActual("../../web3/hooks/useTBTCv2TokenContract") as {}),
useTBTCv2TokenContract: jest.fn(),
}))

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you double-check if all the tests pass? For me they don't (on your branch) but on main all have passed

Copy link
Collaborator Author

@r-czajkowski r-czajkowski Apr 14, 2023

Choose a reason for hiding this comment

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

Once we merge main should be all good 🤞

`DefaultProtocolHistory` -> `ProtocolHistory`
Clarify in comment that the `timestamp` field is in seconds.
We should write `TBTC` (started with capital `T`) when it's in the
middle of the word that is writtne in camelCase convention. Everywhere
else we should stick to `tBTC`.
`TBTCBrdigeStats` -> `TBTCBridgeStats`
@github-actions
Copy link

Copy link
Contributor

@michalsmiarowski michalsmiarowski left a comment

Choose a reason for hiding this comment

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

LGTM 🚋

@michalsmiarowski michalsmiarowski merged commit 254dc1a into main Apr 19, 2023
@michalsmiarowski michalsmiarowski deleted the tbtc-stats-on-overview-page branch April 19, 2023 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Links to recent deposits in dApp Show the recent network activity in dApp to reassure new users
3 participants