cln-application icon indicating copy to clipboard operation
cln-application copied to clipboard

Display Invoice Rune

Open evansmj opened this issue 1 year ago • 2 comments

This commit adds an invoice rune to the ConnectWallet component. Most values in that screen come from the backend method /shared/connectwallet and the type is found at app-config.type.ts/WalletConnect.

An invoice rune is added to WalletConnect because that is what is shown on the ConnectWallet.tsx component. ApplicationActions.SET_WALLET_CONNECT appends an invoice rune if one is found. If one is not found, ConnectWallet.tsx has a useEffect that checks for a missing invoice rune, and makes a createrune rpc call and then refreshes the component.

evansmj avatar May 01 '24 16:05 evansmj

I refactored sendRequestToSetStore to take Request as a parameter, so that I could bundle api calls that use different urls or methods, and send them to the same callback, such as for:

sendRequestToSetStore(
      appCtx.setWalletConnect, 
      { method: 'get', url: '/shared/connectwallet' },
      { method: 'post', url: '/cln/call', body: { method: 'showrunes' } });

evansmj avatar May 01 '24 18:05 evansmj

Hi @evansmj, Thank you for the PR.

Here are my inputs after reviewing it:

  • Lint errors: There are two lint errors (use-http.ts Line 114 & ConnectWallet.tsx Line 51). The first one can be removed by adding missing getConnectWallet dependency in the array. For connectwallet missing dependency error, adding // eslint-disable-next-line react-hooks/exhaustive-deps will disable it for the linter.

  • New Invoice rune on every refresh: Currently createInvoiceRune is not dependent upon showrunes response; resulting in a new invoice rune creation on every refresh/startup. I ended up with 120 new invoice rune while testing this PR :).

  • Generating a rune without user action: As runes are directly related to node security, it would be better to generate it with user's active involvement. For that, we can leave the box empty if the rune doesn't exist already with Add button. And generate the rune only if user clicks on the button (image below).

  • Showrunes: Calling showrunes in the frontend will expose all runes from the node to the browser. I would avoid calling it in the backend also as we are already using another file (env COMMANDO_CONFIG) to get the master rune from. We can save INVOICE_RUNE along with LIGHTNING_PUBKEY and LIGHTNING_RUNE in the same file.

  • Invoice rune persistance: Currently, newly generate invoice rune only persists in the frontend. It would be better if we can keep the state persistent in backend & frontend both.

In summary, invoice rune user story should look like:

1: On application start, read the INVOICE_RUNE value from COMMANDO_CONFIG. 2: Set the value in backend's CONNECT_WALLET_SETTINGS either by step 1 or set to empty string if missing. 3: Show invoice rune value on the connect wallet screen. Show Add button with tooltip if it is empty otherwise simple copy button. 4: On Add button click, call create rune from the frontend. 5: Once the rune value in received in the backend, save that in the COMMANDO_CONFIG. 6: Also set it in the backend (APP_CONSTANTS?, CONNECT_WALLET_SETTINGS?, lightning.service.ts's constructor(), etc.). 7: Send the whole connectWallet response back to the UI and update the UI with Invoice rune value.

Screenshot from 2024-05-16 16-17-21

ShahanaFarooqui avatar May 16 '24 23:05 ShahanaFarooqui

Hey @ShahanaFarooqui im looking through figma and the existing app but i can't find a nice + icon svg. Is there one you have in mind? otherwise i have this one:

Screenshot 2024-05-27 at 9 36 37 PM Screenshot 2024-05-27 at 9 36 47 PM

evansmj avatar May 28 '24 01:05 evansmj

It looks fine but I would recommend to tag Basak on figma asking for plus icon svg. It will save us some time and reworking effort.

ShahanaFarooqui avatar May 28 '24 01:05 ShahanaFarooqui

It looks fine but I would recommend to tag Basak on figma asking for plus icon svg. It will save us some time and reworking effort.

Sure i've added the request here https://www.figma.com/design/09CUihPE1P6CDf7KlqTExu?node-id=203-2197#818167883

evansmj avatar May 28 '24 01:05 evansmj

hey @ShahanaFarooqui ready for review. I also added a loading state and disable the invoice rune buttons while loading. And I show toasts for successfully creating an invoice rune and if an error occurs.

evansmj avatar May 31 '24 02:05 evansmj

Thanks everyone. Looking forward to integrating CLN soon!

john-zaprite avatar Jun 06 '24 15:06 john-zaprite