feat: render non-vbank NFTs in offers #125

samsiegart · 2023-09-14T23:15:27Z

Closes #116

cloudflare-workers-and-pages · 2023-09-14T23:18:55Z

Deploying with Cloudflare Pages

Latest commit:	`7ff97c0`
Status:	✅ Deploy successful!
Preview URL:	https://b826a1c5.wallet-app.pages.dev
Branch Preview URL:	https://nft-wallet-app.wallet-app.pages.dev

View logs

wallet/src/components/Proposal.tsx

wallet/src/components/SmartWalletConnection.tsx

wallet/src/service/Offers.ts

turadg · 2023-09-15T18:04:17Z

wallet/src/service/Offers.ts

-          const value = String(entry.value);
-          return [kw, { pursePetname, value }];
-        }),
+        await Promise.all(


this looks like an opportunity to simplify with objectMap and deeplyFulfilledObject. I think with that you would need no await

I tried playing around with this but couldn't get it working. If we have time, I'd love to pair on this

check out 04ae1c6

I think that refactoring makes sense to apply here too, but not a blocker

wallet/src/service/Offers.ts

turadg · 2023-09-15T18:05:26Z

wallet/src/util/WalletBackendAdapter.ts

@@ -402,7 +403,13 @@ export const makeWalletBridgeFromFollowers = (
      // If we ever add non 'set' amount purses that aren't in the vbank, it's
      // not currently possible to read their decimalPlaces, so this code
      // will need updating.
-      if (!Array.isArray(purse.balance.value)) {
+
+      const isCopyBag = Object.prototype.hasOwnProperty.call(


we must have something already in Endo to check this. if not, please file an issue.

Indeed
https://github.com/endojs/endo/blob/66bb24c0226add70fcb6dc01c55418f7582e7c0b/packages/patterns/index.js#L5

I tried using isCopySet, but it kept returning false. Maybe I'm not passing in the correct data. I looked at how the method was used in other projects and couldn't discern what object shape is expects to be passed in

try https://github.com/endojs/endo/blob/66bb24c0226add70fcb6dc01c55418f7582e7c0b/packages/patterns/index.js#L9C3-L9C12

turadg · 2023-09-19T18:08:45Z

wallet/src/service/Offers.ts

+import { M } from '@agoric/vat-data';
+import { mustMatch } from '@agoric/store';


available in Endo now

Suggested change

import { M } from '@agoric/vat-data';

import { mustMatch } from '@agoric/store';

import { M, mustMatch } from '@endo/patterns';

turadg · 2023-09-19T18:09:46Z

wallet/src/service/Offers.ts

@@ -41,6 +44,41 @@ type GiveOrWantEntries = {
  };
 };

+const CapDataShape = harden({ body: M.string(), slots: M.array() });


This looks right but is a concern of Endo. If it's not available in Endo please file an issue for Endo to provide it.

endojs/endo#1782

turadg · 2023-09-19T18:10:01Z

wallet/src/service/Offers.ts

@@ -41,6 +44,41 @@ type GiveOrWantEntries = {
  };
 };

+const CapDataShape = harden({ body: M.string(), slots: M.array() });
+
+const theOne = <T>(array: Array<T> | undefined) => {


consider putting this in a utils module. eventually we might want it in @agoric/internal but it's pretty lightweight to copy around

I don't think this abstraction is appropriate for this use-case, explained in my comment below. Is this motivated rather by some other code in the sdk?

You're right. I misread the error conditions for #125 (comment)

There are other cases where an array length other than 1 is an error, and the utility is useful for those, but this spot want undefined in that case.

I'll file an issue in agoric-sdk to have this added to @agoric/internal

Agoric/agoric-sdk#8358

turadg · 2023-09-19T18:11:09Z

wallet/src/service/Offers.ts

+const theOne = <T>(array: Array<T> | undefined) => {
+  assert(array?.length === 1);
+
+  const first = array.at(0);


surprising to see at

Suggested change

const first = array.at(0);

const first = array[0];

but actually, the first element could very well be undefined. so,

Suggested change

const first = array.at(0);

return array[0];

turadg · 2023-09-19T18:13:32Z

wallet/src/util/WalletBackendAdapter.ts

@@ -402,7 +403,13 @@ export const makeWalletBridgeFromFollowers = (
      // If we ever add non 'set' amount purses that aren't in the vbank, it's
      // not currently possible to read their decimalPlaces, so this code
      // will need updating.
-      if (!Array.isArray(purse.balance.value)) {
+
+      const isCopyBag = Object.prototype.hasOwnProperty.call(


try https://github.com/endojs/endo/blob/66bb24c0226add70fcb6dc01c55418f7582e7c0b/packages/patterns/index.js#L9C3-L9C12

turadg · 2023-09-19T18:43:47Z

wallet/src/service/Offers.ts

-          const value = String(entry.value);
-          return [kw, { pursePetname, value }];
-        }),
+        await Promise.all(


check out 04ae1c6

samsiegart · 2023-09-19T19:24:16Z

wallet/src/service/Offers.ts

+    ),
+  ]);
+
+  const brandPetname = theOne(brands.find(([_, brand]) => brand === brand));


Did you test this before pushing? brands is an array of tuples of [petname, brand], so assert(array?.length === 1); above would always throw since the length is 2. I think it's better to leave the ?.at(0) and leave the brandPetname undefined if missing, then show an error in the component that renders the offer so that the user doesn't have to open the console to know something went wrong. Additionally, I think throwing will break the loop that watches pending offers, preventing it from rendering other offers.

Thanks for pointing this out. Turns out the purse was created, so I was no longer executing this code path

samsiegart · 2023-09-19T19:39:39Z

wallet/src/service/Offers.ts

@@ -41,6 +44,41 @@ type GiveOrWantEntries = {
  };
 };

+const CapDataShape = harden({ body: M.string(), slots: M.array() });
+
+const theOne = <T>(array: Array<T> | undefined) => {


I don't think this abstraction is appropriate for this use-case, explained in my comment below. Is this motivated rather by some other code in the sdk?

turadg · 2023-09-20T16:48:06Z

wallet/src/components/PurseValue.tsx

@@ -191,7 +192,7 @@ const PurseValue = ({ value, displayInfo, brandPetname }) => {
  const isSet = displayInfo?.assetKind === AssetKind.SET;
  const isCopyBag = displayInfo?.assetKind === AssetKind.COPY_BAG;

-  if (isCopyBag && Object.prototype.hasOwnProperty.call(value, 'payload')) {
+  if (isCopyBag && endoIsCopyBag(value)) {


I wonder why one would be true and not the other. Is that exceptional?

I wasn't sure if there was some legacy situation where we could have a copyBag, but it didn't have the correct payload. @samsiegart, do you think this is a safe change?

turadg · 2023-09-20T16:48:19Z

wallet/src/service/Offers.ts

+              );
+              const purse = brandToPurse.get(amount.brand);
+
+              // if (purse) {


remove disabled code

turadg · 2023-09-20T16:48:41Z

wallet/src/service/Offers.ts

              await getBrandPetNameAndDisplayInfoWithoutPurse(
                watcher,
                amount.brand,
-              );
+                );
+                console.log("GYT")


turadg · 2023-09-20T16:51:15Z

wallet/src/util/WalletBackendAdapter.ts

@@ -402,7 +404,10 @@ export const makeWalletBridgeFromFollowers = (
      // If we ever add non 'set' amount purses that aren't in the vbank, it's
      // not currently possible to read their decimalPlaces, so this code
      // will need updating.
-      if (!Array.isArray(purse.balance.value)) {
+
+      const isCopyBag = endoIsCopyBag(purse.balance.value);


that it comes from Endo isn't semantically meaningful. please omit from the name.

I think the bar should be high for renaming an import so I'd suggest keeping the imported name and using a different name for the boolean.

otherwise import the function as something meaningful like isCopyBagValue.

samsiegart · 2023-09-20T18:21:32Z

wallet/src/service/Offers.ts

+  watcher: ChainStorageWatcher,
+  brand: Brand,
+) => {
+  const brandsP = watcher.queryOnce<[string, unknown][]>([


This used to be in the top-level of offer service so it's queried one time, but now it's queried for every proposal entry. Consider moving it back so it can be memoized.

samsiegart · 2023-09-20T18:21:34Z

wallet/src/service/Offers.ts

+
+  const [brands, boardAux] = await Promise.all([
+    brandsP,
+    watcher.queryBoardAux<{


Similarly here, consider memoizing the boardAux for each brand.

mhofman · 2023-09-20T18:21:57Z

wallet/src/service/Offers.ts

@@ -109,26 +141,34 @@ export const getOfferService = (
      return Object.fromEntries(entries);
    };

+    const readDisplayInfo = async (entry: PurseDisplayInfo) => {
+      mustMatch(harden(entry.amount), CapDataShape, 'amount');


Why enforce the shape here instead of deferring to the marshaller what it will accept?

turadg

No blockers. I trust you'll use "squash" to merge.

turadg · 2023-09-20T20:12:38Z

wallet/src/service/Offers.ts

-          const value = String(entry.value);
-          return [kw, { pursePetname, value }];
-        }),
+        await Promise.all(


I think that refactoring makes sense to apply here too, but not a blocker

Co-authored-by: Samuel Siegart <[email protected]>

samsiegart force-pushed the nft-wallet-app branch 2 times, most recently from 41b024f to ea7eb7b Compare September 15, 2023 04:51

iomekam force-pushed the nft-wallet-app branch 2 times, most recently from d37cc23 to 0471b92 Compare September 15, 2023 16:59

turadg requested changes Sep 15, 2023

View reviewed changes

iomekam requested a review from turadg September 19, 2023 16:49

turadg reviewed Sep 19, 2023

View reviewed changes

samsiegart commented Sep 19, 2023

View reviewed changes

iomekam requested a review from turadg September 20, 2023 16:38

turadg requested changes Sep 20, 2023

View reviewed changes

samsiegart commented Sep 20, 2023

View reviewed changes

mhofman reviewed Sep 20, 2023

View reviewed changes

iomekam requested a review from turadg September 20, 2023 20:08

turadg approved these changes Sep 20, 2023

View reviewed changes

feat: render non-vbank NFTs in offers

7ff97c0

iomekam force-pushed the nft-wallet-app branch from bf91ffe to 7ff97c0 Compare September 21, 2023 02:33

iomekam merged commit fcf2b73 into main Sep 21, 2023

iomekam deleted the nft-wallet-app branch September 21, 2023 02:41

iomekam added a commit that referenced this pull request Sep 21, 2023

feat: render non-vbank NFTs in offers (#125) (#126)

03c1244

Co-authored-by: Samuel Siegart <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: render non-vbank NFTs in offers #125

feat: render non-vbank NFTs in offers #125

samsiegart commented Sep 14, 2023 •

edited by iomekam

Loading

cloudflare-workers-and-pages bot commented Sep 14, 2023 •

edited

Loading

turadg Sep 15, 2023

iomekam Sep 19, 2023

turadg Sep 19, 2023

turadg Sep 20, 2023

turadg Sep 15, 2023

erights Sep 16, 2023

iomekam Sep 19, 2023

turadg Sep 19, 2023

turadg Sep 19, 2023

turadg Sep 19, 2023

iomekam Sep 20, 2023

turadg Sep 19, 2023

samsiegart Sep 19, 2023

turadg Sep 19, 2023

iomekam Sep 20, 2023

iomekam Sep 20, 2023

turadg Sep 19, 2023

turadg Sep 19, 2023

turadg Sep 19, 2023

samsiegart Sep 19, 2023

iomekam Sep 20, 2023

samsiegart Sep 19, 2023

turadg Sep 20, 2023

iomekam Sep 20, 2023

turadg Sep 20, 2023

turadg Sep 20, 2023

turadg Sep 20, 2023

samsiegart Sep 20, 2023

samsiegart Sep 20, 2023

mhofman Sep 20, 2023

turadg left a comment

turadg Sep 20, 2023

		import { M } from '@agoric/vat-data';
		import { mustMatch } from '@agoric/store';

	import { M } from '@agoric/vat-data';
	import { mustMatch } from '@agoric/store';
	import { M, mustMatch } from '@endo/patterns';

feat: render non-vbank NFTs in offers #125

feat: render non-vbank NFTs in offers #125

Conversation

samsiegart commented Sep 14, 2023 • edited by iomekam Loading

cloudflare-workers-and-pages bot commented Sep 14, 2023 • edited Loading

Deploying with Cloudflare Pages

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

Choose a reason for hiding this comment

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samsiegart commented Sep 14, 2023 •

edited by iomekam

Loading

cloudflare-workers-and-pages bot commented Sep 14, 2023 •

edited

Loading