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

feat: calculate withdrawal address for keplr [OTE-875] #1182

Conversation

yogurtandjam
Copy link
Contributor

move cosmosChainAddresses to useMemo and export it from useTransfers hook

use cosmosChainAddresses to calculate cosmos address.
if we dont receive an address from cosmosChainAddresses, then we're not connected to keplr

@yogurtandjam yogurtandjam requested a review from a team as a code owner October 19, 2024 02:16
Copy link

linear bot commented Oct 19, 2024

Copy link

vercel bot commented Oct 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
v4-staging ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 21, 2024 8:44pm
v4-testnet ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 21, 2024 8:44pm

Comment on lines +120 to +124
// Cosmos chains connect to the keplr wallet, which has a unique address per chain id
const calculatedCosmosAddress = toChainId && cosmosChainAddresses[toChainId];
setToAddress(calculatedCosmosAddress ?? sourceAccount.address);
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm so instead of setting all of these initial values in a useEffect, these can just be initialized as initial state in useTransfer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 absolutely agree, i had a comment somewhere about doing this but it probably got lost.
i have a ticket here: https://linear.app/dydx/issue/OTE-869/optimize-usetransfers to track this work

ideally rn would like to separate PR's by goal though. i feel like once all the functionality is in it will be easier to write the best pattern to optimize this, rather than trying to guess what it should all look like before we have all the features in.

i'll add a TODO comment with this ticket linked to make sure we take care of this if that works for you

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm you could probably do in #1155 directly since that's where you create the WithdrawForm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i was hoping to start the deposit form first - cause i think it actually might make the most sense to split the useTransfers hook into useWithdrawals useDeposits so we don't have to flag everything on TransferType but i'd want to work through all the gotchas and usecases first to make sure it's the proper refactor

@yogurtandjam yogurtandjam merged commit 49f53db into jerms/OTE-855_cctp-withdrawals-5 Oct 22, 2024
7 checks passed
@yogurtandjam yogurtandjam deleted the jerms/OTE-875_default-withdrawal-address-keplr branch October 22, 2024 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants