Payments are virtual objects. #2526

katelynsills · 2021-02-24T01:17:19Z

First attempt to try to use virtual objects, just for payments in issuer.js in ERTP.

TODO:

#dapp-fungible-faucet-branch: prepare-test-env
#dapp-card-store-branch: prepare-test-env
#dapp-otc-branch: prepare-test-env
#dapp-oracle-branch: prepare-test-env
#dapp-pegasus-branch: prepare-test-env
#dapp-encouragement-branch: prepare-test-env
#dapp-simple-exchange-branch:prepare-test-env
#dapp-svelte-wallet-branch: prepare-test-env
#dapp-token-economy-branch: prepare-test-env
#documentation-branch: prepare-test-env

FUDCo · 2021-02-24T07:31:27Z

packages/ERTP/src/payment.js

+      self: Far(makeFarName(allegedName, ERTPKind.PAYMENT), {
+        getAllegedBrand: () => state.brand,
+      }),


Not sure how well the Far(makeFarName(... stuff is going to get along with the virtual object machinery. The construct is certainly ugly and very hard to parse visually. I would have written this as just:

self: { getAllegedBrand: () => state.brand, }

But if this object is going to be used as a presence we need to get that Far() wrapper on there somehow.

The return value from paymentVOMaker is going to get hardened. I don't know how harden deals with objects with properties that are already hardened. I think that's OK, but this is one of those mysterious edge cases that I always have to either ask @erights about or just try and see what happens.

harden is idempotent. That part is fine.

I was expecting the Far... to be in the makeKind invocationL

const paymentMaker = makeKind(Far(makeFarName(allegedName, ERTPKind.PAYMENT), paymentVOMaker);

I don't like needing the self: section. There was a slightly different pattern that didn't require that, but I couldn't lay my hands on that issue yet.

As @dtribble said, harden() is idempotent. However Far is not: it only accepts unhardened objects (it mutates them before hardening them).

I have a branch in which makeKind asserts that the maker it was given is applying Far to the self that it returns, under the theory that the maker should be doing harden to the { init, self } pair that it returns ("don't let objects out of your sight without hardening them first"), and Far() cannot be applied to objects that are already hardened, therefore the maker must apply the Far before it hardens { init, self }.

(the assertion on that branch isn't very helpful, however, because if you make the most common mistake and don't harden the return value, it emits a particularly weird error message instead of something that would guide you towards the light)

It would certainly be more ergonomic to tell makeKind the interface name, and then have makeKind apply the Far() itself, but that would require that the maker not harden its return value, which feels counter to the habit we're trying to teach developers.

I think that integration is the right thing. It should happen within the same module. The init/self is the squishy underbelly of constructing useful remote and persistent objects. They don't need to be hardened, given that they should never escape directly.

(Retracted previous comment. I took "escape" out of context)

Oh good. To clarify for the future, I just meant that the internal description of what needs to be built -- paymentVOMaker -- on line 9 gets used on line 18 and the result of that is what gets locked down by the process. The paymentVOMaker should never escape.

packages/ERTP/src/payment.js

katelynsills · 2021-02-27T00:31:39Z

Brought in the Fake Virtual Object Manager from #2543 and was able to get it to work in AVA tests! I'm now getting errors like the following, which I think might be a bug in how the virtual object version of WeakStore (or maybe the mocked version?) is treating the objects. It seems to expect a string key instead.

  unitTests › mintObj › mint.mintPayment default nat MathKind

  Rejected promise returned by test. Reason:

  TypeError {
    message: '(an object) must be a string',
  }

  › parseVatSlot (packages/SwingSet/src/parseVatSlots.js:37:16)
  › fakeConvertSlotToVal (packages/SwingSet/tools/fakeVirtualObjectManager.js:39:31)
  › fullRevive (packages/marshal/src/marshal.js:970:39)
  › fullRevive (packages/marshal/src/marshal.js:1018:28)
  › Object.unserialize (packages/marshal/src/marshal.js:1042:19)
  › Object.get (packages/SwingSet/src/kernel/virtualObjectManager.js:282:20)
  › packages/ERTP/src/issuer.js:201:30
  › async packages/ERTP/test/unitTests/test-mintObj.js:15:27

dckc · 2021-03-01T20:18:43Z

... Ask @dckc for help in getting makeKind and makeWeakStore as globals in XS tests

looks like packages/ERTP/test/_setupFakeVirtualObjectManager.js should get handled like SESboot SESboot https://github.com/Agoric/agoric-sdk/blob/master/packages/xsnap/src/avaXS.js#L16

the tricky bit there is rolling up the import. I did it in a build step for SESboot. budleSource might work, though. Oh... and having avaXS depend on ERTP or SwingSet is awkward. hm. That suggests some sort of command-line option to inject scripts, like ava. Hm.

also, these two globals should get added to the test compartment endowments https://github.com/Agoric/agoric-sdk/blob/master/packages/xsnap/src/avaHandler.js#L59 .

Here's hoping I can get to this after a SES release today...

dckc · 2021-03-02T20:06:45Z

I checked with Kate and we agreed it's best for me to push directly to this branch...

dckc · 2021-03-05T17:29:56Z

packages/zoe/src/contractFacet/contractFacet.js

@@ -32,6 +34,8 @@ import '../../exported';
 import '../internal-types';

 export function buildRootObject(powers, _params, testJigSetter = undefined) {
+  console.log('makeKind in zcf', makeKind());
+  console.log('makeWeakStore in zcf', makeWeakStore());


are these console.log statements temporary? I usually mark my temporary stuff with '@@'.

or is this advice out of date?

In general we need to remove unsolicited console.logs from our codebase
-- https://github.com/Agoric/agoric-sdk/wiki/agoric-sdk-unit-testing#patterns

Temporary for debugging purposes. This is a draft

katelynsills · 2021-03-09T00:28:43Z

@dckc, in order to get linting to pass, I had to make some tiny changes to avaXS.js. Does this look ok to you? q was already declared above, but I didn't know what else to call it, so I did qq:

.

dckc · 2021-03-09T06:00:08Z

@dckc, in order to get linting to pass, I had to make some tiny changes to avaXS.js. Does this look ok to you?

Yes. I think I made similar changes on another branch so we may have merge conflicts coming...

Chris-Hibbert

Looks good. I skipped over xsnap/src/avaXS.js, presuming someone else (@dckc ?) looked at it closely.

One nit about duplicated names.

Chris-Hibbert · 2021-03-09T18:57:20Z

packages/ERTP/test/swingsetTests/basicFunctionality/vat-alice.js

+    makeAliceMaker() {
+      return harden(makeAliceMaker(vatPowers.testLog));


can these two makeAliceMaker functions have different names? Maybe the inner one is makeAliceMakerActual?

erights

To understand this PR well requires me to understand out external store stuff more than I currently do. I definitely need to dive deeper into that. However, I do understand it well enough to see that this PR correctly uses it, so there's no need to delay this PR on that.

makeKind is an endowment for accessing an external store. I understand why we might want to make this an ambient endowment. But why makeWeakStore? And why makeWeakStore and not makeStore?

Placing them is the general eslint config so that they can be used without a /* global declaration makes me uncomfortable. I would like to discuss this before this PR is merged. If we cannot resolve this quickly, it looks to me like that change could be split off from this PR. With that removed, I'm happy to LGTM this PR. Note: I may well have agreed to this global listing earlier, so I understand the frustration if I'm reversing myself. But that's part of why we review. Once implemented, an idea can look less pleasant than one imagined.

It seems that there are two makeWeakStores, our earlier vanilla one and the new external one. Again, I think I may have been the one to suggest that they be unified this way, but I would like to revisit this. However, IIUC, this specific issue does not need to delay this PR. It is more an issue with what has already been merged as part of the external store system.

I did not review any of the tests. If there are particular tests that you would especially like me to look at, please let me know.

I did not review the XS Ava changes. Please get @dckc 's reaction to those.

I'm submitting this review as "Comment" rather than "Request changes" because I don't know that changes are called for. Rather, we need to discuss.

dckc · 2021-03-12T06:53:00Z

The avaXS.js changes basically made it into another branch, so I should be able to resolve those conflicts easily tomorrow.

katelynsills · 2021-03-12T19:37:49Z

The avaXS.js changes basically made it into another branch, so I should be able to resolve those conflicts easily tomorrow.

I can just rebase.

erights

@katelynsills and I discussed. The only change needed in this PR is not to extend the eslint config list of assumed globals.

All my other concerns are with the external store abstractions that are already merged, which I need to investigate more deeply anyway, and separately. Attn @FUDCo

LGTM, thanks!

katelynsills · 2021-03-12T19:59:00Z

makeKind is an endowment for accessing an external store. I understand why we might want to make this an ambient endowment. But why makeWeakStore? And why makeWeakStore and not makeStore?....
...It seems that there are two makeWeakStores, our earlier vanilla one and the new external one.

@FUDCo, I think these are questions for you. Should I copy this over into an issue for further discussion with @rights?

Placing them is the general eslint config so that they can be used without a /* global declaration makes me uncomfortable.

As discussed, I will remove from the eslint config, and use the /* global declaration instead, but I will keep them in the Typescript globals because Typescript doesn't understand the /* global declarations.

katelynsills · 2021-03-12T19:59:50Z

Jinx :)

katelynsills · 2021-03-12T22:03:23Z

@dckc This was the one xsnap change that remained after rebasing - should it be in this PR? Anything I missed that should be here? Thanks!

dckc · 2021-03-13T00:10:43Z

@dckc This was the one xsnap change that remained after rebasing - should it be in this PR? Anything I missed that should be here? Thanks!

The ava-xs shell script got replaced by an ava-xs.js file. So that shell script should not be in this PR.

katelynsills requested review from warner and FUDCo February 24, 2021 01:17

katelynsills self-assigned this Feb 24, 2021

katelynsills added Beta ERTP package: ERTP labels Feb 24, 2021

katelynsills modified the milestones: Beta Governance Launch, Beta Initial Launch Feb 24, 2021

katelynsills requested a review from dtribble February 24, 2021 01:18

FUDCo reviewed Feb 24, 2021

View reviewed changes

katelynsills commented Feb 24, 2021

View reviewed changes

packages/ERTP/src/payment.js Outdated Show resolved Hide resolved

katelynsills mentioned this pull request Feb 24, 2021

Possible bug in Virtual Objects #2533

Closed

katelynsills force-pushed the 1830-ERTP-virtual-objects-payments branch 2 times, most recently from c3982b7 to f9cb176 Compare February 27, 2021 00:28

warner mentioned this pull request Mar 1, 2021

Implement a mock virtual object manager to support unit tests outside SwingSet #2543

Merged

katelynsills removed the request for review from dtribble March 1, 2021 18:44

katelynsills force-pushed the 1830-ERTP-virtual-objects-payments branch from f9cb176 to 3adcc6a Compare March 1, 2021 20:00

dckc self-assigned this Mar 2, 2021

katelynsills mentioned this pull request Mar 3, 2021

declarative environment import module #2571

Closed

katelynsills force-pushed the 1830-ERTP-virtual-objects-payments branch 2 times, most recently from 76e249f to f5d8a0a Compare March 5, 2021 01:09

dckc reviewed Mar 5, 2021

View reviewed changes

katelynsills force-pushed the 1830-ERTP-virtual-objects-payments branch from 342eaec to 582982b Compare March 5, 2021 19:11

dckc removed their assignment Mar 8, 2021

katelynsills force-pushed the 1830-ERTP-virtual-objects-payments branch from 7f7d604 to 7ec916c Compare March 8, 2021 22:15

katelynsills marked this pull request as ready for review March 9, 2021 00:19

katelynsills changed the title ~~DRAFT: payments are virtual objects.~~ Payments are virtual objects. Mar 9, 2021

katelynsills requested a review from erights March 9, 2021 00:25

katelynsills requested a review from Chris-Hibbert March 9, 2021 01:28

Chris-Hibbert approved these changes Mar 9, 2021

View reviewed changes

erights reviewed Mar 12, 2021

View reviewed changes

erights approved these changes Mar 12, 2021

View reviewed changes

katelynsills force-pushed the 1830-ERTP-virtual-objects-payments branch from 7a7b08a to 34f3379 Compare March 12, 2021 21:58

katelynsills mentioned this pull request Mar 12, 2021

refactor(ERTP): use jessie in issuer.js #2606

Merged

warner mentioned this pull request Mar 15, 2021

increase swingset DEFAULT_VIRTUAL_OBJECT_CACHE_SIZE #2644

Open

katelynsills force-pushed the 1830-ERTP-virtual-objects-payments branch from 34f3379 to 022a488 Compare March 16, 2021 17:46

refactor: ERTP payments as virtual objects

e6a0924

katelynsills force-pushed the 1830-ERTP-virtual-objects-payments branch from 022a488 to e6a0924 Compare March 16, 2021 17:48

katelynsills enabled auto-merge (squash) March 16, 2021 17:48

katelynsills merged commit bc36651 into master Mar 16, 2021

katelynsills deleted the 1830-ERTP-virtual-objects-payments branch March 16, 2021 18:02

katelynsills mentioned this pull request Mar 22, 2021

try rewriting issuer.js with Representatives/Containers (virtual objects) #1830

Closed

dckc mentioned this pull request Feb 8, 2022

Virtual objects and virtual stores for ERTP #4504

Closed

Kashuhackerone mentioned this pull request Oct 22, 2022

[Snyk] Security upgrade @agoric/cosmic-swingset from 0.19.0 to 0.26.0 Kashuhackerone/agoric-sdk#137

Open

Kashuhackerone mentioned this pull request Jun 14, 2023

[Snyk] Security upgrade @agoric/cosmic-swingset from 0.19.0 to 0.26.0 Kashuhackerone/agoric-sdk#169

Open

	if (payments.length > 1) {
	const paymentSet = new Set();
	payments.forEach(payment => {
	if (paymentSet.has(payment)) {
	throw new Error('same payment seen twice');
	}
	paymentSet.add(payment);
	});

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Payments are virtual objects. #2526

Payments are virtual objects. #2526

katelynsills commented Feb 24, 2021 •

edited

Loading

FUDCo Feb 24, 2021

dtribble Feb 24, 2021

dtribble Feb 24, 2021 •

edited

Loading

warner Feb 24, 2021

dtribble Feb 24, 2021

erights Feb 25, 2021

dtribble Feb 25, 2021

katelynsills commented Feb 27, 2021

dckc commented Mar 1, 2021

dckc commented Mar 2, 2021 •

edited

Loading

dckc Mar 5, 2021

katelynsills Mar 5, 2021

katelynsills commented Mar 9, 2021

dckc commented Mar 9, 2021

Chris-Hibbert left a comment

Chris-Hibbert Mar 9, 2021

erights left a comment •

edited

Loading

dckc commented Mar 12, 2021

katelynsills commented Mar 12, 2021

erights left a comment

katelynsills commented Mar 12, 2021 •

edited

Loading

katelynsills commented Mar 12, 2021

katelynsills commented Mar 12, 2021

dckc commented Mar 13, 2021

		makeAliceMaker() {
		return harden(makeAliceMaker(vatPowers.testLog));

Payments are virtual objects. #2526

Payments are virtual objects. #2526

Conversation

katelynsills commented Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtribble Feb 24, 2021 • edited Loading

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

katelynsills commented Feb 27, 2021

dckc commented Mar 1, 2021

dckc commented Mar 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katelynsills commented Mar 9, 2021

dckc commented Mar 9, 2021

Chris-Hibbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erights left a comment • edited Loading

Choose a reason for hiding this comment

dckc commented Mar 12, 2021

katelynsills commented Mar 12, 2021

erights left a comment

Choose a reason for hiding this comment

katelynsills commented Mar 12, 2021 • edited Loading

katelynsills commented Mar 12, 2021

katelynsills commented Mar 12, 2021

dckc commented Mar 13, 2021

katelynsills commented Feb 24, 2021 •

edited

Loading

dtribble Feb 24, 2021 •

edited

Loading

dckc commented Mar 2, 2021 •

edited

Loading

erights left a comment •

edited

Loading

katelynsills commented Mar 12, 2021 •

edited

Loading