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(ui): support emerynet #62

Merged
merged 4 commits into from
Sep 30, 2024
Merged

feat(ui): support emerynet #62

merged 4 commits into from
Sep 30, 2024

Conversation

samsiegart
Copy link
Contributor

@samsiegart samsiegart commented Sep 26, 2024

fixes #43

  • Adds emerynet to network selector
  • Removes hardcoded agoriclocal/osmosislocal wallet connections and RPC queries
  • Puts RPC addresses and IBC channels in a dictionary indexed by the currently selected network in the network selector

(Unrelated):

  • UI now always uses dark mode because colors are wonky with light mode

Testing

Tested with local instance of multichain-testing, created ICAs on agoric and osmosis and sent IST to both of them. Haven't tested on emerynet yet.

@samsiegart samsiegart changed the title wip(ui): support emerynet feat(ui): support emerynet Sep 27, 2024
@samsiegart samsiegart marked this pull request as ready for review September 27, 2024 18:58
@samsiegart samsiegart requested a review from Jovonni September 27, 2024 19:00
Comment on lines +32 to +40
testChain: {
chainId: 'agoric-emerynet-8',
chainName: 'emerynet',
iconUrl: 'agoric.svg',
},
apis: {
rest: ['https://emerynet.api.agoric.net'],
rpc: ['https://emerynet.rpc.agoric.net'],
},
Copy link
Member

Choose a reason for hiding this comment

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

I ended up using the network config to do some codegen for deploying the contract:

9fe5ab9

I could write a .json file or a .js module with chainId and rest / rpc, if it seems useful. Maybe more trouble than it's worth.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems potentially useful, I guess the output would need to be checked in somewhere though, might as well just keep this here.

@dckc
Copy link
Member

dckc commented Sep 27, 2024

I haven't deployed the contract, but when I yarn dev, choose emerynet, connect my wallet, and hit Create, I get a Please Select Chain thing; the page dims and nothing I click on has any effect.

image

@samsiegart
Copy link
Contributor Author

I haven't deployed the contract, but when I yarn dev, choose emerynet, connect my wallet, and hit Create, I get a Please Select Chain thing; the page dims and nothing I click on has any effect.

The screen is dimming because it's opening an empty modal, below the screenshot it's there and you can dismiss it. I'm not sure what the intended logic with the modal is though, @Jovonni do you have any guidance here?

The "please select chain" makes sense because there's no chain selected in the dropdown. If I select osmosis then try I get the following:
Screen Shot 2024-09-27 at 5 15 48 PM

@dckc
Copy link
Member

dckc commented Sep 28, 2024

The "please select chain" makes sense because there's no chain selected in the dropdown.

Oh! I thought when I picked emerynet I had picked a chain. Now I get it. thanks.

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

I'd approve this but for the regression from { chainName, denom} to { chainName } in the offer.

Comment on lines -17 to -18
"@agoric/react-components": "^0.1.1-dev-ca0ddde.0",
"@agoric/ui-components": "^0.3.9-u13.0",
Copy link
Member

Choose a reason for hiding this comment

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

These 2 deps were just dead code, I suppose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, ui-components wasn't imported anywhere, and web-components was only used to import a type that wasn't used anywhere

agoric: 'http://127.0.0.1:26657',
};

const initializeKeplr = async () => {
Copy link
Member

Choose a reason for hiding this comment

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

👏 nice to see that dapps don't have to do this sort of thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Jovonni Hinted that this might be useful for local dev/debugging stuff (sending tokens around etc.), just by nature of having the network added to your keplr I assume. I guess we could put it back in and only invoke it when you select "Agoric Local" in the dropdown.

Copy link
Contributor

Choose a reason for hiding this comment

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

yea it may come in handy in the near future, depending on what devs end up doing with the dapp. For example, when trying to set up a auto delegate scenario, you have to start with the ibc/denom on agoric-local, then submit it with the offer.

i put a little convenience to help set it up, without having to use the dapp for now, so may not need on the UI:

fund-osmo-ibc:
kubectl exec -i osmosislocal-genesis-0 -c validator -- osmosisd tx ibc-transfer transfer transfer channel-0 ${CLIENTADDR} 1000000uosmo --from faucet --chain-id osmosislocal --node http://127.0.0.1:26657 --fees 5000uosmo -y;
kubectl exec -i osmosislocal-genesis-0 -c validator -- osmosisd tx ibc-transfer transfer transfer channel-1 ${CLIENTADDR} 1000000uosmo --from faucet --chain-id osmosislocal --node http://127.0.0.1:26657 --fees 5000uosmo -y;

@@ -247,22 +194,13 @@ const MakeAccount = () => {
} else {
throw new Error('unsupported address prefix');
}
const { signingClient, address: walletAddress } = walletConnection;
Copy link
Member

Choose a reason for hiding this comment

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

👏

@@ -285,25 +223,13 @@ const MakeAccount = () => {
}
console.log('message sent successfully');
} else {
await window.keplr.enable(`${chain}local`);
Copy link
Member

Choose a reason for hiding this comment

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

👏 bye bye

const sendMsg = {
typeUrl: '/ibc.applications.transfer.v1.MsgTransfer',
value: {
sourcePort: 'transfer',
sourceChannel: 'channel-0',
sourceChannel: ibcChannels[agoricChainName][chain],
Copy link
Member

Choose a reason for hiding this comment

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

ok.

Comment on lines 121 to 119
{ chainName: selectedChain, denom: 'ubld' },
{ chainName: selectedChain }, // adjust as needed
Copy link
Member

Choose a reason for hiding this comment

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

oh... the contract required denom here now.

The codegen stuff I did handles that. Maybe it's worthwhile after all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just put denom back here, didn't mean to remove it initially, I think I rebased wrong

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

This is worth a try.

@samsiegart if some sort of basic test for the network selector would cost less than 20min, please add that before merging. Hm. I don't see any ui tests here at all. So maybe that's not so straightforward.

ui/src/App.tsx Outdated
@@ -11,7 +11,7 @@ function App() {
const { themeClass } = useTheme();

return (
<ThemeProvider>
<ThemeProvider forceColorMode="dark">
Copy link
Contributor

Choose a reason for hiding this comment

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

@amessbee observed an issue with some UI elements showing up on light mode, wondering how this renders. I can try it out real quick

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's kind of tricky because we're using daisyui as well as interchain-ui. This makes interchain-ui dark (including the network dropdown), and the ui/tailwind.config.js change makes daisyui dark (majority of the ui elements).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm it doesn't seem to be working for me though, the network dropdown still looks light, looking into it

Copy link
Contributor

@Jovonni Jovonni left a comment

Choose a reason for hiding this comment

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

LGTM, my comment on the vstorage query doesnt have to block this, but would be nice if we can get this in here. If not I can try to bang it out soon too np

nice work @samsiegart 🚀


export const ibcChannels = {
'agoric-local': {
osmosis: 'channel-1',
Copy link
Contributor

Choose a reason for hiding this comment

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

one issue here is that this isn't always deterministically channel-1, when using multichain-testing.

This would still have the same problem as the UI had before, when the relayers startup in opposite order (aka which relayer finishes setup first gets the first channel-x)

We may eventually solve this at the multichain-testing side. A better approach on my todo was to query the connection from vstorage, and i think you were thinking the same thing @samsiegart ?

Here's the query we can do to get it (the 3rd query here)

agd q vstorage children published.agoricNames.chain
children:
- agoric
- agoriclocal
- celestia
- cosmoshub
- dydx
- juno
- neutron
- noble
- omniflixhub
- osmosis
- secretnetwork
- stargaze
- stride
- umee
pagination: null
contract % agd q vstorage children published.agoricNames.chainConnection
children:
- agoric-3_cosmoshub-4
- agoric-3_noble-1
- agoric-3_omniflixhub-1
- agoric-3_osmosis-1
- agoric-3_secret-4
- agoric-3_stride-1
- agoric-3_umee-1
- agoriclocal_cosmoshub-4
- agoriclocal_gaialocal
- agoriclocal_osmosislocal
- celestia_neutron-1
- celestia_osmosis-1
- celestia_secret-4
- celestia_stargaze-1
- celestia_stride-1
- cosmoshub-4_juno-1
- cosmoshub-4_neutron-1
- cosmoshub-4_noble-1
- cosmoshub-4_omniflixhub-1
- cosmoshub-4_osmosis-1
- cosmoshub-4_secret-4
- cosmoshub-4_stargaze-1
- cosmoshub-4_stride-1
- dydx-mainnet-1_neutron-1
- dydx-mainnet-1_noble-1
- dydx-mainnet-1_osmosis-1
- dydx-mainnet-1_stride-1
- dydx-mainnet-1_umee-1
- gaialocal_osmosislocal
- juno-1_neutron-1
- juno-1_noble-1
- juno-1_osmosis-1
- juno-1_secret-4
- juno-1_stargaze-1
- juno-1_stride-1
- neutron-1_noble-1
- neutron-1_osmosis-1
- neutron-1_secret-4
- neutron-1_stargaze-1
- neutron-1_stride-1
- noble-1_omniflixhub-1
- noble-1_osmosis-1
- noble-1_secret-4
- noble-1_stargaze-1
- noble-1_umee-1
- omniflixhub-1_osmosis-1
- osmosis-1_secret-4
- osmosis-1_stargaze-1
- osmosis-1_stride-1
- osmosis-1_umee-1
- secret-4_stargaze-1
- secret-4_stride-1
- secret-4_umee-1
- stargaze-1_stride-1
- stride-1_umee-1
pagination: null
agd q vstorage data published.agoricNames.chainConnection.agoriclocal_osmosislocal
value: '{"blockHeight":"187","values":["{\"body\":\"{\\\"client_id\\\":\\\"07-tendermint-0\\\",\\\"counterparty\\\":{\\\"client_id\\\":\\\"07-tendermint-1\\\",\\\"connection_id\\\":\\\"connection-1\\\",\\\"prefix\\\":{\\\"key_prefix\\\":\\\"FIXME\\\"}},\\\"id\\\":\\\"connection-0\\\",\\\"state\\\":3,\\\"transferChannel\\\":{\\\"channelId\\\":\\\"channel-0\\\",\\\"counterPartyChannelId\\\":\\\"channel-1\\\",\\\"counterPartyPortId\\\":\\\"transfer\\\",\\\"ordering\\\":0,\\\"portId\\\":\\\"transfer\\\",\\\"state\\\":3,\\\"version\\\":\\\"ics20-1\\\"}}\",\"slots\":[]}"]}'

I'm thinking we can just fetch it when the dapp loads? or dynamically, and keep the channels/connections in memory. WDYT?

thoughts too @dckc

Copy link
Member

Choose a reason for hiding this comment

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

This would still have the same problem as the UI had before ...

Right; this is an issue that pre-dates this PR. Let's leave it out of scope by postponing it to a separate issue:

@samsiegart samsiegart merged commit 63f8535 into main Sep 30, 2024
3 checks passed
@samsiegart samsiegart deleted the emerynet-ui branch September 30, 2024 19:03
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.

UI network selector does not include emerynet
3 participants