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

fix: gateway explorer links #251

Merged
merged 3 commits into from
Nov 6, 2023
Merged

fix: gateway explorer links #251

merged 3 commits into from
Nov 6, 2023

Conversation

okjodom
Copy link
Collaborator

@okjodom okjodom commented Oct 30, 2023

When gateway is deployed on mainnet and testnet, gateway address and node links should point to valid mempool.space links.
On signet, we assume gateway is deployed on mutinynet.com where we point both links. When gateway is on regtest, or the network is not yet known, we don't show "view in explorer" links

Changes in gateway UI take effect after fedimint/fedimint#3455 which introduces network field to gateway info. To validate these changes before the upstream changes modify the network prop in FederationCard and observe how the links are affected. Valid network values are "main" | "test" | "signet" | "regtest"

closes #242

EthnTuttle
EthnTuttle previously approved these changes Oct 30, 2023
Copy link
Collaborator

@EthnTuttle EthnTuttle left a comment

Choose a reason for hiding this comment

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

LGTM.
Pending fedimint/fedimint#3455, will merge.

@okjodom
Copy link
Collaborator Author

okjodom commented Oct 30, 2023

LGTM. Pending fedimint/fedimint#3455, will merge.

Thought to clarify that this change hides the link when the network prop is undefined. IMO makes it okay to merge before upstream lands. When upstream lands and gateways update, the link will be shown per the network configured

Copy link
Collaborator

@wbobeirne wbobeirne left a comment

Choose a reason for hiding this comment

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

As a point of policy, I'm also hesitant to merge changes that remove functionality with the current master of fedimint. I think when we want to target future changes, the game plan should be to maintain backwards compatibility (i.e. default to the mainnet link when !network) but priming the app to be ready for an incoming change when the API updates to get the new response type makes the most sense.

If you've got the time to make it backwards compatible, it'd be good to get into the habit of doing that. Otherwise we could just wait for the fedimint PR to land, there's no value in getting this into a 0.1.x release unless a gateway release is also ported with this network property, which I suspect won't happen.

@@ -69,7 +69,7 @@
"sort": "Sort"
},
"info-card": {
"amboss_node_link_text": "View on amboss.space",
"open-node-in-explorer": "Open in explorer",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not important, but we could change this to View on {{origin}} and update the t calls to include the origin of the link they pulled together. Not really worth holding up the PR, but ideally we avoid deviating from design wherever possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interpolation with origin results in this madness "View on https://mempool.space". Went with View on {{host}}

apps/gateway-ui/src/components/DepositCard.tsx Outdated Show resolved Hide resolved
@okjodom
Copy link
Collaborator Author

okjodom commented Oct 31, 2023

If you've got the time to make it backwards compatible, it'd be good to get into the habit of doing that. Otherwise we could just wait for the fedimint PR to land, there's no value in getting this into a 0.1.x release unless a gateway release is also ported with this network property, which I suspect won't happen.

Updated the change to make it backward compatible in UI as a start

@wbobeirne wbobeirne merged commit 856ba40 into fedimint:master Nov 6, 2023
1 check passed
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.

"view on mempool.space" and "view on amboss" don't work on mutinynet
3 participants