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

Display redemptions in my activity #570

Merged
merged 10 commits into from
Jul 14, 2023

Conversation

r-czajkowski
Copy link
Collaborator

This PR adds support for fetching pending/completed redemptions and displaying them under the my activity table on the Bridge page.

Use builder pattern to build the redemption details link. Also keep the
`buildRedemptionDetailsLink` function to not break the current API. We
need more generic approach to build link because in
`RedemptionRequested` evnet we do not have `btcAddress` but
`redeemerOutputScript` in expected format. Also in event we can find
`walletPublicKeyHash` not `walletPublicKey` - using builder we can build
link from `walletPublicKey` or `walletPublicKeyHash` and `btcAddress` or
`redeemerOutpuScript` and the builder class is responsible for
converting params to correct format.
Take into account requested and completed redemptions to display them in
the `my activity` table on the bridge page.
And update the store data based on emitted data.
We should sort from newest to oldest.
@github-actions
Copy link

1 similar comment
@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.

Overall looking very good 🔥 Left come comments to look at before the merge though:

src/utils/tBTC.ts Outdated Show resolved Hide resolved
export interface BridgeActivity {
bridgeProcess: BridgeProcess
Copy link
Contributor

Choose a reason for hiding this comment

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

TBH from the UI perspective it's hard to know which one of the transaction from bridge activity is mint and which one is unmint if all of them all pending. For example take a look at my Bridge Activity on testnet:

image

Just by looking at it I have no clue which one is which (well, except the mint one). Having that much pending transaction on miannet is probably unlikely to happen but who knows. Maybe we could think about some way to improve the readability. WYT @sashatanase @r-czajkowski ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe instead of PENDING we could display MINTING/UNMINTG ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's update later when we get feedback from the designers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

src/threshold-ts/tbtc/index.ts Show resolved Hide resolved
src/threshold-ts/tbtc/index.ts Outdated Show resolved Hide resolved
.sort((a, b) => b.blockNumber - a.blockNumber)
}

private _findDepositActivities = async (
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just personal opinion so feel free to ignore it but i believe we could name it _findDepositBridgeActivities. When there will be a situation when we go back to this project after some months then I think that name will immedietely indicate what is this method for where using just activities might be confusing. But like I said, just my personal preference. We can keep that up too if you prefer it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe we should rename to findDepositHistory and findRedemptionHistory?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, I think we should get rid of the bridgeActivity from the TBTC interface because it's too specific for the dapp. I think we should fetch this data in hook. I believe we had a conversation about it some time ago.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes i agree. The bridgeHistory might work better in our code and it won't be specific to the dApp naming. I'll leave it to you to decide if you want ot change it or not. I also like the names you proposed - findDepositHistory and findRedemptionHistory. We can leavi it for now too if you want and refactor it later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would rename/remove it in a separate PR to make sure we do(not) want this method in TBTC interface and deal with it once and for all.

src/threshold-ts/tbtc/index.ts Outdated Show resolved Hide resolved
src/threshold-ts/tbtc/index.ts Outdated Show resolved Hide resolved
src/threshold-ts/tbtc/index.ts Show resolved Hide resolved
src/hooks/tbtc/useSubscribeToRedemptionRequestedEvent.ts Outdated Show resolved Hide resolved
}
}

function findRedemptionActivity(
Copy link
Contributor

@michalsmiarowski michalsmiarowski Jul 14, 2023

Choose a reason for hiding this comment

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

If you agree with #570 (comment) then we might also rename this one to findRedemptionBridgeActivity

List missed params in error message.
Since we now gather both deposit and remption data it seems that
`depositor` name for the `bridgeAcivity` parameter doesn't really match
and might be misleading. Here we rename param to just `account`.
Use array instead of map. While working on this, I thought getting
redemption by key would be useful, but it looks like it's unnecessary
now.
@github-actions
Copy link

Add utils functions that help convert number to token precision.
@github-actions
Copy link

Point out that `activityKey` stores the redemption key for redemption
and deposit key for deposit.
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

Comment on lines 39 to 41
// TODO: Take into account fees, see
// https://github.com/threshold-network/token-dashboard/pull/569
amount: fromSatoshiToTokenPrecision(requestedAmount).toString(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not forget about this TODO

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@github-actions
Copy link

There is no need to take into account fees in this case because in the
`my activity` table we want to display the tBTC amount that user
requested to unmint - they actually decrease their balance of tBTC token
by this amount.
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 3dd89f7 into main Jul 14, 2023
5 checks passed
@michalsmiarowski michalsmiarowski deleted the display-redemptions-in-my-activity branch July 14, 2023 13:10
@github-actions
Copy link

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.

2 participants