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

New Tab: IBC #795

Merged
merged 5 commits into from
Mar 20, 2024
Merged

New Tab: IBC #795

merged 5 commits into from
Mar 20, 2024

Conversation

grod220
Copy link
Collaborator

@grod220 grod220 commented Mar 19, 2024

Closes #248

Does not address design, just functional elements. Design to come next. Updates:

  • Replaces governance for new IBC tab
  • Adds logic about what assets can be ibc-withdrawn
  • Add validation checks (via new bech32 functions)
  • Add tx details page for ics20 withdraws

Comment on lines +162 to +170
const validateAddress = (chain: Chain | undefined, address: string): boolean => {
if (!chain || address === '') return false;
return bech32IsValid(address, chain.addressPrefix);
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Valid:
Screenshot 2024-03-19 at 3 51 35 PM

Invalid:
Screenshot 2024-03-19 at 3 51 41 PM

Copy link
Member

Choose a reason for hiding this comment

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

Can we allow any address if the chain is undefined?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This particular check where chain can be undefined relates to zustand booting up empty and then populating with chain data from our hard-coded registry. We can't transfer without a channel-id being available first.

import { joinLoHiAmount } from '@penumbra-zone/types/src/amount';
import { bech32Address } from '@penumbra-zone/bech32';

export const Ics20WithdrawalComponent = ({ value }: { value: Ics20Withdrawal }) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ics20Withdraw's do not have a special private/public case fyi

Screenshot 2024-03-19 at 3 52 15 PM

Copy link
Member

Choose a reason for hiding this comment

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

Not immediately but we probably do want to have a view for the Ics20Withdrawal so that we can insert:

  • token metadata
  • address view (of return address)

@Valentine1898
Copy link
Contributor

Valentine1898 commented Mar 19, 2024

I get this error when trying to enter amount greater than 2, while I have a note with amount 2.
It looks like I can't spend more than one note with IBC withdrawal

telegram-cloud-photo-size-2-5462977910232177415-x

@grod220
Copy link
Collaborator Author

grod220 commented Mar 20, 2024

@Valentine1898 let's pair on that error you're seeing

@grod220 grod220 force-pushed the ibc-page branch 2 times, most recently from 40644a0 to b027fdf Compare March 20, 2024 11:26
Comment on lines +36 to +37
<div className='grow overflow-auto p-[30px] pb-44 pt-10'>
<p className='bg-text-linear bg-clip-text pb-2 font-headline text-2xl font-bold text-transparent'>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixes bug where tx approval window does not scroll all the way down given approve/deny buttons overlap it with a fixed position

Comment on lines +13 to +18
<div>
{/* eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing */}
{memoView.value.plaintext?.text || (
<span className='italic text-gray-600'>== none ==</span>
)}
</div>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was confused by seeing &nbsp; in the text box. So added a bit of styling instead:

Screenshot 2024-03-20 at 12 29 07 PM

Copy link
Contributor

@Valentine1898 Valentine1898 left a comment

Choose a reason for hiding this comment

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

Good job 👍, all LGTM

@grod220 grod220 merged commit 00fc134 into main Mar 20, 2024
6 checks passed
@grod220 grod220 deleted the ibc-page branch March 20, 2024 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ics20Withdraw part 2
3 participants