feat(wallet-connection): Connect dapp directly to wallet UI #5750

samsiegart · 2022-07-12T06:44:52Z

We can now add the useLocalStorage={true} property to the wallet connection component to get the dapp to talk directly to the wallet UI in local storage, that way the dapp doesn't directly depend on an ag-solo connection.

samsiegart · 2022-07-13T05:14:58Z

packages/solo/public/wallet-bridge.html

-        window.parent.postMessage({ type: 'walletBridgeLoaded' }, '*');
-      }
-      retryWebSocket();
+      window.addEventListener('load', () => {


This ensures the message wont be posted until after the iframe's "onLoad" event fires. This is the way it works with the react bridge.jsx, so we can rely on the consistent ordering of events in the WalletConnection component.

dckc

seems pretty good, but given the low test coverage for this stuff, I'd like to give @turadg a chance to look it over

turadg · 2022-07-13T12:48:55Z

docs/env.md

+Until dApps are converted to connect to the smart-wallet UI directly,
+this allows them to continue to connect to `/wallet-bridge.html` and such
+on the solo and have these endpoints serviced by `/wallet/bridge.html`
+and such in a wallet UI.


Wasn't clear to me from reading this what the purpose was (the difference between the two paths or how it worked.)

Consider:

This enables a proxy so that the solo bridge interface (/wallet-bridge.html) is backed by the smart wallet (/wallet/bridge.html). Dapps designed for the solo bridge can enable this until they connect to the smart wallet directly.

turadg · 2022-07-13T12:51:17Z

packages/solo/public/wallet-bridge.html

-        window.parent.postMessage({ type: 'walletBridgeLoaded' }, '*');
-      }
-      retryWebSocket();
+      window.addEventListener('load', () => {


your PR comment would make a good code comment.

Suggested change

window.addEventListener('load', () => {

// This ensures the message wont be posted until after the iframe's "onLoad" event fires

// so we can rely on the consistent ordering of events in the WalletConnection component.

window.addEventListener('load', () => {

yes, please!

packages/solo/src/web.js

packages/wallet/ui/config-overrides/index.js

turadg · 2022-07-13T12:57:52Z

packages/wallet/ui/public/bridge.html

+</head>
+
+<body style="overflow-x: hidden; overflow-y: scroll">
+    <noscript>Without JavaScript enabled, this page cannot function.</noscript>


that statement is true invariantly. minor but let's tell the user something about the state.

Suggested change

<noscript>Without JavaScript enabled, this page cannot function.</noscript>

<noscript>ERROR: JavaScript not available.</noscript>

Fair enough. Better to be explicit and tell them how to fix it?

I have a hard time blaming the user when the page requires JavaScript. I was trying to concisely say something like "Please excuse the requirement for you to put yourself at risk by enabling JavaScript in your browser. This page uses JavaScript for more than progressive enhancement. Providing similar functionality without JavaScript is not practical."

Just used the same message as index.html. Do people still use the web without javascript? At the very least they're trying to use a dapp so I don't think we need to be apologetic.

... so I don't think we need to be apologetic.

No, we don't need to, but...

Do people still use the web without javascript?

Yes. In fact, I bet you do it regularly, with curl and the like.

turadg · 2022-07-13T13:30:49Z

packages/wallet/ui/src/bridge.jsx

+      );
+  } else {
+    console.debug('bridge: SKIP document.requestStorageAccess');
+    conn.connectStorage(window.localStorage);


could this fail?

No, I don't think so. It only calls addEventListener; I don't see how that could fail.

I've been bitten by localStorage permission problems, but I suppose if you've got the object you won't find out about any errors until mutating. And even then you could have failures later with hitting quotas, the permission being revoked, etc. So fine to handle those cases later.

turadg · 2022-07-13T13:37:13Z

packages/wallet/ui/src/components/WalletConnection.jsx

@@ -28,7 +30,17 @@ const WalletConnection = ({
  connectionConfig,
 }) => {
  const classes = useStyles();
-  const [wc, setWC] = useState(null);
+  /**
+   * ISSUE: where to get the full types for these?


e.g. for WalletConnection at this line to the bottom of AgoricWalletConnection.js,

/** @typedef {InstanceType<AgoricWalletConnection>['walletConnection']} FarWalletConnection */

I suggest "Far" to distinguish from the other "WalletConnection" strings.

I couldn't get this working

turadg · 2022-07-13T13:38:00Z

packages/wallet/ui/src/components/WalletConnection.jsx

+      localStorage: storage,
+      addEventListener,
+      removeEventListener,
+    } = window; // WARNING: ambient


consider using window. at each access so the ambience is apparent at the callsite

I'd rather go the other direction: pass in these parts of window explicitly rather than using window ambiently.

OTOH, window.addEventListener is quite likely more conventional and easier to read, and since the current code doesn't follow ocap discipline, maybe making it easier to read is better.

Switched to window.

turadg · 2022-07-13T13:40:10Z

packages/wallet/ui/src/components/WalletConnection.jsx

+      removeEventListener,
+    } = window; // WARNING: ambient
+
+    function handleStorageMessage(key, newValue) {


a lot of logic in here that's not UI concern. consider extracting some to a utility function. that could be unit tested.

Pulled this into a separate function. I don't know how much of this code is just placeholder/PoC until after we have the smart wallet bridge fully working, so I won't bother writing tests yet.

turadg · 2022-07-13T13:41:31Z

packages/wallet/ui/src/lockdown.js

+// eslint-disable-next-line no-restricted-properties
+const { pow: mathPow } = Math;
+// eslint-disable-next-line no-restricted-properties
+Math.pow = (base, exp) =>
+  typeof base === 'bigint' && typeof exp === 'bigint'
+    ? base ** exp
+    : mathPow(base, exp);


do we not have a shim that can be called to do this?

Umm... This is such a shim, no?

I mean one that can be imported here.

I'm pretty sure this is a one-off. It's kind of a kludge, really.

It's not a new kludge, btw. It comes from...

agoric-sdk/packages/wallet/ui/public/index.html

Lines 12 to 15 in 39fe092

Math.pow = (base, exp) =>

typeof base === 'bigint' && typeof exp === 'bigint'

? base ** exp

: mathPow(base, exp);

Git says that's from @samsiegart Oct 2021 01a9118 but I suspect it predates that too.

Ah. Where is it used? I can't find any instances. If there are any consider making them bigint-safe there instead of modifying Math.

I don't know where it's used. Maybe some 3rd party library?

I sure wish we had made a test.

Yes this code has been here longer than I have, will leave this convo open but won't block this PR on it.

dckc

It's at least a step in the right direction.

turadg

- document SOLO_BRIDGE_TARGET

it worked up to making an offer; the trade didn't complete... not sure why - much wrestling with webpack magic in config-overrides to add wallet/bridge.html entry point - move lockdown() call from .html to .jsx - HandledPromise identity discontinuity musings

- JSON.stringify is more familiar

* chore(web-components): revert casting experiment * feat(solo): transitional reverse-proxy wallet-bridge - document SOLO_BRIDGE_TARGET * feat(wallet/api): E(walletAdmin).getScopedBridge() * build(wallet/ui): deps += react-app-rewire-multiple-entry * feat(wallet/ui): localStorage bridge functional in AMM (WIP) it worked up to making an offer; the trade didn't complete... not sure why - much wrestling with webpack magic in config-overrides to add wallet/bridge.html entry point - move lockdown() call from .html to .jsx - HandledPromise identity discontinuity musings * chore(wallet/ui): prune bridge.html to essentials * doc(casting): package name typo in README * refactor(wallet/ui): take advantage of ev type - JSON.stringify is more familiar * chore: sameParent is evidently superfluous * chore: delay storage listener until user requests * chore: gate storage connection on requestStorageAccess * chore: sameParent check before postMessage. noop??? * feat: use correct bridge url * feat: amm swap working * chore: prettier * fix: address feedback * chore: rebase Co-authored-by: Dan Connolly <connolly@agoric.com>

samsiegart changed the title ~~Sam bridge working~~ Working AMM Swap Jul 12, 2022

samsiegart force-pushed the sam-bridge-working branch 3 times, most recently from 0580cac to 16d853c Compare July 13, 2022 05:04

samsiegart changed the title ~~Working AMM Swap~~ feat(wallet-connection): Connect dapp directly to wallet UI Jul 13, 2022

samsiegart requested a review from dckc July 13, 2022 05:10

samsiegart marked this pull request as ready for review July 13, 2022 05:10

samsiegart commented Jul 13, 2022

View reviewed changes

dckc requested a review from turadg July 13, 2022 05:22

dckc reviewed Jul 13, 2022

View reviewed changes

turadg reviewed Jul 13, 2022

View reviewed changes

samsiegart requested review from turadg and dckc July 14, 2022 03:11

dckc approved these changes Jul 14, 2022

View reviewed changes

turadg approved these changes Jul 14, 2022

View reviewed changes

dckc and others added 15 commits July 14, 2022 09:34

chore(web-components): revert casting experiment

23397b2

feat(solo): transitional reverse-proxy wallet-bridge

16b153a

- document SOLO_BRIDGE_TARGET

feat(wallet/api): E(walletAdmin).getScopedBridge()

6f84447

build(wallet/ui): deps += react-app-rewire-multiple-entry

9a15c3e

chore(wallet/ui): prune bridge.html to essentials

99e920d

doc(casting): package name typo in README

b17d4b7

refactor(wallet/ui): take advantage of ev type

0110b47

- JSON.stringify is more familiar

chore: sameParent is evidently superfluous

90b89df

chore: delay storage listener until user requests

49ad042

chore: gate storage connection on requestStorageAccess

3e8e76d

chore: sameParent check before postMessage. noop???

454e918

feat: use correct bridge url

01fa1d6

feat: amm swap working

17eb1c8

chore: prettier

bd23daa

samsiegart added 2 commits July 14, 2022 09:34

fix: address feedback

1cf29d7

chore: rebase

688cc2b

samsiegart force-pushed the sam-bridge-working branch from fff0237 to 688cc2b Compare July 14, 2022 16:51

samsiegart added the automerge:squash Automatically squash merge label Jul 14, 2022

mergify bot merged commit 1dd584b into master Jul 14, 2022

mergify bot deleted the sam-bridge-working branch July 14, 2022 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(wallet-connection): Connect dapp directly to wallet UI #5750

feat(wallet-connection): Connect dapp directly to wallet UI #5750

samsiegart commented Jul 12, 2022 •

edited

Loading

samsiegart Jul 13, 2022

dckc left a comment

turadg Jul 13, 2022

samsiegart Jul 13, 2022

turadg Jul 13, 2022

dckc Jul 13, 2022

samsiegart Jul 13, 2022

turadg Jul 13, 2022

dckc Jul 13, 2022

samsiegart Jul 13, 2022

dckc Jul 14, 2022

turadg Jul 13, 2022

dckc Jul 13, 2022

turadg Jul 13, 2022

turadg Jul 13, 2022

samsiegart Jul 14, 2022

turadg Jul 13, 2022

dckc Jul 13, 2022

samsiegart Jul 14, 2022

turadg Jul 13, 2022

dckc Jul 13, 2022

samsiegart Jul 14, 2022

turadg Jul 13, 2022

dckc Jul 13, 2022

turadg Jul 13, 2022

dckc Jul 13, 2022

dckc Jul 13, 2022

turadg Jul 13, 2022

dckc Jul 13, 2022

samsiegart Jul 13, 2022

dckc left a comment

turadg left a comment

	<noscript>Without JavaScript enabled, this page cannot function.</noscript>
	<noscript>ERROR: JavaScript not available.</noscript>

	Math.pow = (base, exp) =>
	typeof base === 'bigint' && typeof exp === 'bigint'
	? base ** exp
	: mathPow(base, exp);

feat(wallet-connection): Connect dapp directly to wallet UI #5750

feat(wallet-connection): Connect dapp directly to wallet UI #5750

Conversation

samsiegart commented Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

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

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

dckc left a comment

Choose a reason for hiding this comment

turadg left a comment

Choose a reason for hiding this comment

samsiegart commented Jul 12, 2022 •

edited

Loading