feat: ibc transfers #63

samsiegart · 2023-03-14T04:27:04Z

Fixes: #50

WARNING

This component always attempts to make transactions on mainnet, regardless of if the UI config is set to a local/test network.

Testing

Connected to local chain with same keplr account that has tokens on mainnet. Typed setPreviewEnabled(true) in the console. Sent real tokens over IBC and saw that my balances were updated in Keplr.

TODO (Done)

Since this isn't testable outside of production, we can do one of the following:

Hide this feature behind a feature flag, and/or only show it if mainnet is selected, to avoid confusion.
Keep this as a draft until completion of allow agoric run add-collateral-core.js to work without a solo agoric-sdk#7104 (which at this time is not estimated). We'll then likely have to change the implementation to not rely on wallet/src/util/ibc-assets.ts so arbitrary networks and denoms can be tested.

I'm leaning towards choice 1 because it's much less work (we could call this basically done from here) and we'll have to verify that it works in mainnet anyway.

cloudflare-workers-and-pages · 2023-03-14T04:29:49Z

Deploying with Cloudflare Pages

Latest commit:	`5cd04af`
Status:	⚡️ Build in progress...

View logs

samsiegart · 2023-03-14T04:33:01Z

@turadg @michaelfig @dckc Please have a read of the PR description and let me know your thoughts. I'm also willing to discuss in real-time if helpful.

dckc

nice to see this taking shape!

I haven't managed to really try it out.

I think the assertIsDeliverTxSuccess call is critical.

The rest are just suggestions to think about.

dckc · 2023-03-14T07:25:10Z

wallet/src/components/IbcTransfer.tsx

+import { SigningStargateClient } from '@cosmjs/stargate';
+import { CircularProgress, Link, Snackbar, Typography } from '@mui/material';
+
+export enum IbcDirection {


I'd rather we stuck to the erase-to-js subset of ts and avoid enum

That does go for agoric-sdk because we want to avoid a build step. Browser apps will always have a build step so for these repos we (frontend developers) decided to allow enum.

dckc · 2023-03-14T07:26:01Z

wallet/src/util/well-known-petnames.ts

@@ -0,0 +1,3 @@
+export const wellKnownPetnames = {
+  IbcATOM: 'ATOM',


I expect the vbank name in production to be ATOM.

We can/should change that in our main configs.

Just changed everything to reference 'ATOM' and do the 'IbcATOM' -> 'ATOM' conversion in the beginning when the purse is loaded.

dckc · 2023-03-14T07:26:59Z

wallet/src/util/ibc-assets.ts

+      denom:
+        'ibc/BA313C4A19DFBF943586C0387E6B11286F9E416B4DD27574E6909CABE0E342FA',


the denom should be available via agoricNames.vbankAssets. I suppose what's deployed doesn't line up, so I can see why you would hard-code this.

Yea, maybe we can remove this in favor of that later, but for now this is needed for testing

please an XXX comment to that effect

dckc · 2023-03-14T07:28:07Z

wallet/src/util/ibc-assets.ts

+    chainInfo: {
+      chainName: 'Cosmos Hub',
+      chainId: 'cosmoshub-4',
+      rpc: 'https://cosmoshub-rpc.stakely.io/',


I wonder about fail-over. I suppose stakely's service does some of that internally, but it's still a single point of failure.

Yes good point, it would be good to use an RPC we have some sort of relationship with, ideally an SLA.

agree but not a blocker. please file a ticket for future work (e.g.: fall back if Stakely LB is down)

We need autodiscovery of rpc nodes, and client qualification and selection from the chain registry. Any other approach is not ideal. If we're going to hardcode something, perhaps polkachu's node

not entirely sure I understand autodiscovery, but since we need it...

autodiscovery of rpc nodes #64

Seems like polkachu is locked down:

Access to fetch at 'https://cosmos-rpc.polkachu.com/' from origin 'http://localhost:3000' has been blocked by CORS policy: Request header field content-type is not allowed by Access-Control-Allow-Headers in preflight response.

Do you have any pointers to how autodiscovery of rpc nodes could work?

Since this has it's own ticket I'm not considering it to be a blocker for "feat: ibc transfers"

dckc · 2023-03-14T07:29:27Z

wallet/src/util/Icons.ts

@@ -2,6 +2,7 @@ export const icons = {
  IST: 'tokens/IST.png',
  BLD: 'tokens/BLD.svg',
  ATOM: 'tokens/cosmos.svg',
+  IbcATOM: 'tokens/cosmos.svg',


might as well kludge whatever code reads from agoricNames.vbankAssets to you can avoid this sort of thing

dckc · 2023-03-14T07:43:39Z

wallet/src/components/IbcTransfer.tsx

+        close();
+        showSnackbar(
+          <>
+            Successfully executed transaction{' '}


Looking at sendIbcTokens, it doesn't seem to do assertIsDeliverTxSuccess(tx) so I think we need to do that before declaring success.

dckc · 2023-03-14T07:44:31Z

wallet/src/components/IbcTransfer.tsx

+        setInProgress(false);
+      }
+    } else {
+      /* Is Withdrawal */


comments like this suggest breaking out a named function

Done, but kept the comment

dckc · 2023-03-14T07:47:57Z

wallet/src/components/IbcTransfer.tsx

+    try {
+      const { prefix } = fromBech32(remoteChainAddress);
+      return prefix !== ibcAsset?.chainInfo.addressPrefix;
+    } catch (e) {


lint rules allow unused vars without a special name like _e?

Apparently, just fixed this instance though

dckc · 2023-03-14T07:51:06Z

wallet/src/components/IbcTransfer.tsx

+      try {
+        const res = await signer.sendIbcTokens(
+          remoteChainAddress,
+          keplrConnection.address,


Are we reacting to the user changing to a different account?

This always uses the agoric address that the wallet UI config is currently using. If the user switches keplr accounts after initially loading the wallet UI, I don't know if we handle that well currently, they'd probably have to refresh the page. Otherwise any transactions would just fail to sign.

they'd probably have to refresh the page

IOU an issue.

react to change of account/address #65

dckc · 2023-03-14T07:52:10Z

wallet/src/components/IbcTransfer.tsx

+            Successfully executed transaction{' '}
+            <Link
+              color="rgb(0, 176, 255)"
+              href={`https://bigdipper.live/agoric/transactions/${res.transactionHash}`}


The url seems a little buried; hoist it up near const agoricRpc = ...?

Moved this url into a single util function so it's not repeated. It's a bit tricky to use a const because of the interpolation.

turadg · 2023-03-14T16:09:41Z

I'm leaning towards choice 1 because it's much less work (we could call this basically done from here) and we'll have to verify that it works in mainnet anyway.

+1 to this choice and rationale. Long lived PRs tend to rot.

dckc · 2023-03-14T17:22:34Z

feature flag works for me.

samsiegart · 2023-03-14T19:00:07Z

wallet/src/components/Purses.tsx

  };

  const Purse = purse => {
+    const shouldShowIbcTransferButtons =
+      (keplrConnection?.chainId === agoricChainId || previewEnabled) &&


We can type setPreviewEnabled(true) in the console to enable ibc transfer, otherwise it only shows for mainnet.

consider putting that comment into the code. for non-mainnet, what are we previewing? Is there a ticket to support IBC transfer on other nets?

I guess you could say we're previewing a dangerous misleading version of the component that only makes transactions on mainnet. Just made the comment to that effect.

The closest we have to a ticket is probably Agoric/agoric-sdk#7104. But that doesn't seem to capture everything involved with spinning up two chains, making sure the UI has all the references it needs, etc.

we're previewing a dangerous misleading version of the component that only makes transactions on mainnet

Why would we "preview" that? I think we'd need to "preview" some end user feature that will be production-ready eventually and right now isn't. I haven't seen a ticket for the functionality that will be in production at some point so this sounds more like a "development" or "testing" mode flag.

Will leave this to your judgement.

Yea that is a good point. I just don't know if it's worth making a new flag for this case, maybe just rename it to debug or something...

samsiegart · 2023-03-14T19:05:10Z

I'm able to test this locally because the address for an agoric local chain and mainnet is the same for a given keplr account. So, in real life (prod), I sent tokens to an agoric address. Then, with that keplr account, I created a smart wallet on my local agoric network so that it has the same address. The only caveat is that, in the component, the agoric balance reflects what's on my local chain, not prod. But, the cosmos hub balance reflects cosmos mainnet, and signing the transactions works (but signs on mainnet not local net).

turadg · 2023-03-14T19:21:27Z

wallet/src/components/IbcTransfer.tsx

+import Petname from './Petname';
+import type { PurseInfo } from '../service/Offers';
+import type { KeplrUtils } from '../contexts/Provider';
+import type { Petname as PetnameType } from '@agoric/smart-wallet/src/types';


since Petname is type that spans repos, consider letting it win this naming collision. Petname.tsx could be PetnameSpan.

turadg · 2023-03-14T19:22:52Z

wallet/src/components/IbcTransfer.tsx

+      ibcAsset.chainInfo.chainId,
+    );
+    const accounts = await offlineSigner.getAccounts();
+    setRemoteChainAddress(accounts[0].address);


why is it safe to pick the first? does this code assume the array has only one element? if so best to assert.

Yea keplr only returns one address. I think an assert might be too harsh but added a warning so we can be aware if some behavior changes, it shouldn't break anything though.

turadg · 2023-03-14T19:23:58Z

wallet/src/components/IbcTransfer.tsx

+    setRemoteChainSigner(offlineSigner);
+  };
+
+  const isRemoteChainAddressInvalid = useMemo(() => {


it's more common to check for validity. consider flipping this.

turadg · 2023-03-14T19:27:20Z

wallet/src/components/IbcTransfer.tsx

+    explorerPath: string,
+    transactionHash: string,
+  ) => {
+    setSnackbarMessage(


no need to put this into state. isSnackbarOpen is a signal for whether to render.

Suggested change

setSnackbarMessage(

snackbarMessage = <>

I'm sorry I don't follow. Are you saying we can use an empty message (maybe null) as a signal rather than tracking isSnackbarOpen separately?

No, the opposite. Since we have the Boolean state for whether to present a message we don't also need the message itself to be in state. Just make it a let binding in scope.

Ah I see, because we only change the content when we change isSnackbarOpen, so it will re-render from that. It just seems a bit hazardous to me in case we ever want the content to change while open, simpler to just keep state in useState.

Also, using null content to hide the snackbar I realized will make the hide animation look choppy.

content to change while open

Yeah if that's a requirement then the content should be in state.

using null content to hide the snackbar I realized will make the hide animation look choppy

Oh, I didn't realize that. Why?

Since when you set isSnackbarOpen to false it doesn't disappear immediately, it fades out over a short duration. So, while it's fading, the content will disappear, making the layout shift.

ah thanks for explaining. the current code makes sense. maybe worth some comments though with this context.

Just put all the snackbar stuff in a hook, also added a close button instead of relying on clickaway. Also I combined the state into one useState so that we only call setState once, since it's from an async event this was causing react to render twice unnecessarily.

turadg · 2023-03-14T19:31:06Z

wallet/src/components/IbcTransfer.tsx

+      } else {
+        // Is withdrawal.


shouldn't need a comment. consider else if (direction === IbcDirection.Withdrawal) and an else for unknown values. alternately switch(direction) and call helper functions

Added an else if

turadg · 2023-03-14T19:33:11Z

wallet/src/components/IbcTransfer.tsx

+            <CircularProgress size={36} sx={{ p: 1, mx: 1 }} />
+          ) : (
+            <>
+              {/* @ts-expect-error 'cancel' is part of our theme */}


time to put it into the types?

I'm not sure, we'd have to wrap the component and override the MUI types somewhere down the inheritance tree. It's a big pain that MUI doesn't allow custom color names.

turadg · 2023-03-14T19:34:01Z

wallet/src/components/Purses.tsx

  };

  const Purse = purse => {
+    const shouldShowIbcTransferButtons =
+      (keplrConnection?.chainId === agoricChainId || previewEnabled) &&


consider putting that comment into the code. for non-mainnet, what are we previewing? Is there a ticket to support IBC transfer on other nets?

turadg · 2023-03-14T19:37:34Z

wallet/src/util/ibc-assets.ts

+    chainInfo: {
+      chainName: 'Cosmos Hub',
+      chainId: 'cosmoshub-4',
+      rpc: 'https://cosmoshub-rpc.stakely.io/',


agree but not a blocker. please file a ticket for future work (e.g.: fall back if Stakely LB is down)

turadg · 2023-03-14T19:38:06Z

wallet/src/util/ibc-assets.ts

+      denom:
+        'ibc/BA313C4A19DFBF943586C0387E6B11286F9E416B4DD27574E6909CABE0E342FA',


please an XXX comment to that effect

wallet/src/util/ibcTransfer.ts

dckc · 2023-03-14T20:48:25Z

wallet/src/components/IbcTransfer.tsx

+
+        close();
+        try {
+          assertIsDeliverTxSuccess(res);


may be more straightforward to use...

export declare function isDeliverTxSuccess(result: DeliverTxResponse): boolean;

Done, thanks!

request addressed

turadg · 2023-03-15T17:19:45Z

wallet/src/components/Purses.tsx

+
+interface Props {
+  purses: PurseInfo[] | null;
+  previewEnabled: boolean;


surprised to see this passed along since it's a global state.

Passing it as a prop from context allows components to reactively update when you type setPreviewEnabled(true) in the console.

turadg · 2023-03-15T17:22:16Z

wallet/src/components/Purses.tsx

  };

  const Purse = purse => {
+    const shouldShowIbcTransferButtons =
+      (keplrConnection?.chainId === agoricChainId || previewEnabled) &&


we're previewing a dangerous misleading version of the component that only makes transactions on mainnet

Why would we "preview" that? I think we'd need to "preview" some end user feature that will be production-ready eventually and right now isn't. I haven't seen a ticket for the functionality that will be in production at some point so this sounds more like a "development" or "testing" mode flag.

Will leave this to your judgement.

turadg · 2023-03-15T17:22:57Z

wallet/src/util/ibc-assets.ts

+    chainInfo: {
+      chainName: 'Cosmos Hub',
+      chainId: 'cosmoshub-4',
+      rpc: 'https://cosmoshub-rpc.stakely.io/',


Since this has it's own ticket I'm not considering it to be a blocker for "feat: ibc transfers"

feat: ibc transfers

c483f92

samsiegart closed this Mar 14, 2023

samsiegart reopened this Mar 14, 2023

samsiegart marked this pull request as draft March 14, 2023 04:29

samsiegart requested review from turadg, michaelfig and dckc March 14, 2023 04:31

fix: remove old tests

3b6a850

dckc previously requested changes Mar 14, 2023

View reviewed changes

feat: error toast, feature flag, and refactor

fb94499

samsiegart marked this pull request as ready for review March 14, 2023 18:58

samsiegart requested a review from dckc March 14, 2023 18:58

samsiegart commented Mar 14, 2023

View reviewed changes

turadg reviewed Mar 14, 2023

View reviewed changes

dckc reviewed Mar 14, 2023

View reviewed changes

dckc self-requested a review March 14, 2023 20:48

dckc mentioned this pull request Mar 14, 2023

autodiscovery of rpc nodes #64

Open

samsiegart added 7 commits March 14, 2023 15:24

fix: fix unmodifiable address color

786a9ca

chore: comments

f8c0e62

chore: rename petname component

f82b930

chore: clarify keplr api assumptions

2284911

chore: reverse boolean and explicit conditional

7c54c9b

chore: redundant property key

0b45a33

chore: use util function instead of assert

326a27c

samsiegart requested a review from turadg March 14, 2023 23:11

turadg approved these changes Mar 15, 2023

View reviewed changes

fix: use snackbar hook and eliminate double render

5cd04af

samsiegart merged commit 42c3236 into main Mar 16, 2023

samsiegart deleted the ibc-transfer branch March 16, 2023 04:53

dckc mentioned this pull request Apr 12, 2023

feat(agoric-cli): add ibc-setup and ibc-relayer commands with docs Agoric/agoric-sdk#7065

Merged

		@@ -0,0 +1,3 @@
		export const wellKnownPetnames = {
		IbcATOM: 'ATOM',

		denom:
		'ibc/BA313C4A19DFBF943586C0387E6B11286F9E416B4DD27574E6909CABE0E342FA',

feat: ibc transfers #63

feat: ibc transfers #63

Conversation

samsiegart commented Mar 14, 2023 • edited Loading

WARNING

Testing

TODO (Done)

cloudflare-workers-and-pages bot commented Mar 14, 2023 • edited Loading

Deploying with Cloudflare Pages

samsiegart commented Mar 14, 2023

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arirubinstein Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

dckc Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg commented Mar 14, 2023

dckc commented Mar 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samsiegart commented Mar 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samsiegart commented Mar 14, 2023 •

edited

Loading

cloudflare-workers-and-pages bot commented Mar 14, 2023 •

edited

Loading

arirubinstein Mar 14, 2023 •

edited

Loading

dckc Mar 14, 2023 •

edited

Loading

turadg Mar 15, 2023 •

edited

Loading