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

Add connected uma/wallet screens, show uma in button, persist uma in local storage #119

Conversation

bsiaotickchong
Copy link
Contributor

@bsiaotickchong bsiaotickchong commented Aug 23, 2024

  • with uma, balance permission, spending limit, renewal, and expiration date
    Screenshot 2024-08-23 at 4.17.07 PM.png

  • no uma:
    Screenshot 2024-08-23 at 4.20.45 PM.png

  • no balance permission:

Screenshot 2024-08-23 at 4.24.02 PM.png

  • no spending limit:
    Screenshot 2024-08-23 at 4.24.26 PM.png

  • no expiration:
    Screenshot 2024-08-23 at 4.26.00 PM.png

Copy link
Contributor Author

bsiaotickchong commented Aug 23, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @bsiaotickchong and the rest of your teammates on Graphite Graphite

@bsiaotickchong bsiaotickchong mentioned this pull request Aug 23, 2024
Base automatically changed from 08-21-Save_uma_with_ to develop August 26, 2024 15:53
@bsiaotickchong bsiaotickchong force-pushed the 08-23-Add_connected_uma/wallet_screens_show_uma_in_button_persist_uma_in_local_storage branch from 8f84962 to 3f09615 Compare August 30, 2024 18:14
@bsiaotickchong bsiaotickchong force-pushed the 08-23-Add_connected_uma/wallet_screens_show_uma_in_button_persist_uma_in_local_storage branch from 3f09615 to c398805 Compare August 30, 2024 18:14
Copy link
Contributor

@jklein24 jklein24 left a comment

Choose a reason for hiding this comment

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

Very exciting stuff!!


export const ConnectionCard = ({ connection, uma, balance }: Props) => {
const handleCopyUma = () => {
navigator.clipboard.writeText(uma || "");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do the little "copied" notification thing we do on cn2 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah i think so, or just a change in icon/animation will probably work so it's not as heavy. Will ask design

Comment on lines +31 to +32
// const discoveryDocument = await fetch(`https://${umaDomain}/.well-known/uma-configuration`);
// const discoveryDocumentJson = await discoveryDocument.json<DiscoveryDocument>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to just do this now if you've got the code written out? Pinkdrink works with this now at least, I think? I assume it was just making testing easy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I didn't know it was already being served, i'll fix it in a followup (and i need to try running the backend locally)

authorization_endpoint: "http://localhost:3000/apps/new",
};

// TODO: get real client_id from vasp?
Copy link
Contributor

Choose a reason for hiding this comment

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

The client ID doesn't come from the VASP, it needs to be passed in by the client application as a configuration. They should set their client app identity npub and identity relay in a configuration somewhere.

const clientId =
"npub1u2zfe9zpq2gcuduxatqa5k3alq5yny9qdeq8yrjfqxh4qywa6ejq8daqq6 wss://nos.lol";

// TODO: generate redirectUri based on current page or have it provided by client app
Copy link
Contributor

Choose a reason for hiding this comment

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

I would guess we want to allow client apps to set this themselves, but maybe you're right we could default to the current window.location.href.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah i'm assuming they might change the query params esp if it's a checkout page or something

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah exactly - probably need to have some state there.

kind="secondary"
externalLink={
uma
? `https://nwc.${getUmaDomain(uma)}/connection/${connection.connectionId}`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think instead of using nwc.umadomain here, we might need to load this from the uma-configuration too 🤔. Fine for now, but we'll likely need to update that for flexibility.

@bsiaotickchong bsiaotickchong merged commit 3eb8a4d into develop Aug 31, 2024
6 checks passed
@bsiaotickchong bsiaotickchong deleted the 08-23-Add_connected_uma/wallet_screens_show_uma_in_button_persist_uma_in_local_storage branch August 31, 2024 01:41
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.

2 participants