feat(smart-wallet): trading in well-known non-vbank assets #8071
Conversation
dckc
force-pushed
the
7226-sw-nft-issuer
branch
2 times, most recently
from
c569d5c to
41c25de
Compare
TDD: using a real contract shows we can't deposit NFT payoutsref: test('trading in non-vbank assets', ...) test run log |
smart wallet feature: stumped by "no ordinal" in testmaybe due to using less than a full swingset? Is there some way to use p.s. diagnosis: was using stack trace etc. from test-addAsset.js |
Happy path: trading in non-vbank asset: game real-estate NFTsedit: updated Aug 1 The formerly |
NameHub reverse lookupre I used |
dckc
changed the title
dckc
force-pushed
the
7226-sw-nft-issuer
branch
2 times, most recently
from
2b811eb to
1177b37
Compare
dckc
force-pushed
the
7226-sw-nft-issuer
branch
from
e82b349 to
a711d5a
Compare
dckc
changed the title
dckc
force-pushed
the
7226-sw-nft-issuer
branch
3 times, most recently
from
a7dbcd7 to
721cbbb
Compare
| @@ -65,11 +65,12 @@ harden(assertCapData); | |||
| * @param {Map<string, string>} data | |||
| * @param {string} key | |||
| * @param {ReturnType<typeof import('@endo/marshal').makeMarshal>['fromCapData']} fromCapData | |||
| * @param {number} [index] index of the desired value in a deserialized stream cell | |||
| * @param {number} index index of the desired value in a deserialized stream cell | |||
| @@ -17,6 +17,7 @@ | |||
| }, | |||
| "devDependencies": { | |||
| "@agoric/cosmic-proto": "^0.3.0", | |||
| "@endo/bundle-source": "^2.5.2", | |||
There was a problem hiding this comment.
nit: more of a fixup to the feat commit. if you end up rebasing, consider squashing
| @@ -33,11 +42,14 @@ const bigIntReplacer = (_key, val) => | |||
|
|
|||
| const range = qty => [...Array(qty).keys()]; | |||
|
|
|||
| // We use test.serial() because | |||
| @@ -24,7 +30,10 @@ test.before(async t => { | |||
| space0.produce.bankManager.resolve(bankManager); | |||
| return space0; | |||
| }; | |||
| t.context = await makeDefaultTestContext(t, withBankManager); | |||
| const context = await makeDefaultTestContext(t, withBankManager); | |||
| const bfile = name => new URL(name, import.meta.url).pathname; | |||
There was a problem hiding this comment.
please provide in module scope per recent ocap/tests discussion
There was a problem hiding this comment.
Didn't we agree it's a matter of preference?
There was a problem hiding this comment.
not my recollection. other people will have to maintain this code and import from context when they want to use bfile (and scratch their heads: "why, when this module is not exporting anything?" and then go maybe start another discussion and so on)
… but I'll demote this request to non-blocking
| const bundles = { | ||
| game: await io.bundleSource(io.bfile('./gameAssetContract.js')), | ||
| centralSupply: await io.bundleSource( | ||
| io.bfile('../../vats/src/centralSupply.js'), |
There was a problem hiding this comment.
please use (a local copy of) importSpec so the packages dependency is apparent
| provideBrandToPurses() { | ||
| const brandToPurses = provideLazy( | ||
| walletPurses, | ||
| this.facets.helper, |
There was a problem hiding this comment.
is this our idiom for "the key is this Exo"? is helper identity stable across restarts? and this isn't?
There was a problem hiding this comment.
this is indeed not a stable identity. The idiom is to use one of the facets. Oh... self seems more natural.
| @@ -366,6 +428,87 @@ export const prepareSmartWallet = (baggage, shared) => { | |||
| }, | |||
| }); | |||
| }, | |||
|
|
|||
| provideBrandToPurses() { | |||
There was a problem hiding this comment.
the caller needn't be concerned with provisioning. shouldn't this just be getBrandToPurses?
I'm also wondering whether it's worth putting it on the facet vs just inlining. What else would use this?
I'm genuinely not sure. It is a "helper" method so maybe it doesn't matter much.
There was a problem hiding this comment.
I'm hoisting it to an ordinary function.
inlining obscured the code in providePurseForKnownBrand, but yeah, a facet method is overkill.
| * @param {ERef<NameHub>} known - namehub with brand, issuer branches | ||
| * @returns {Promise<Purse | undefined>} undefined if brand is not well known | ||
| */ | ||
| async providePurseForKnownBrand(brand, known = shared.agoricNames) { |
There was a problem hiding this comment.
The earlier case was more clear-cut, because there was no key argument. But here we do have a key -- the brand. If not here, where would we ever call something provideX? Perhaps only when we're also passing in a store?
There was a problem hiding this comment.
yeah, I think when the caller has the store in which it will be provided. if the caller just wants a thing and doesn't specify where to put it or how to make it, that's just a "get". Similar to how we don't put "memoize" in the name of functions that memoize.
| * | ||
| * We current support only one NameHub, agoricNames, and | ||
| * hence one purse per brand. But we store an array of them | ||
| * to facilitate a transition decentralized introductions. |
| * | ||
| * @param {Brand} brand | ||
| * @param {ERef<NameHub>} known - namehub with brand, issuer branches | ||
| * @returns {Promise<Purse | undefined>} undefined if brand is not well known |
There was a problem hiding this comment.
the method call suggests the caller assumes the brand is well known. should this instead throw when that's not the case?
nit: strike "well" because "how known" is up to the caller
There was a problem hiding this comment.
further down I see that you are concerning this function with "well known" because it takes a remote call that you wouldn't want to have to make before calling this and within the function too.
I think it would help to clarify that in the name, like e.g. providePurseIfKnownBrand. Reading the current name again I see it could be interpreted that way "provide purse for a known brand and undefined otherwise" but the "otherwise" is not hinted at.
There was a problem hiding this comment.
yes, using If in the name is on target.
| if (brandToPurses.has(brand)) { | ||
| const purses = brandToPurses.get(brand); | ||
| if (purses.length > 0) { | ||
| // For now, we have just 1, so use it. |
There was a problem hiding this comment.
| // For now, we have just 1, so use it. | |
| // UNTIL https://github.com/Agoric/agoric-sdk/issues/6126 |
There was a problem hiding this comment.
UNTIL doesn't seem to be documented like XXX, TODO, etc.. oversight?
| } | ||
|
|
||
| /** | ||
| * Introduce an issuer to this wallet. |
There was a problem hiding this comment.
why "introduce" instead of "add"?
naming aside, this is adding an issuer even if it's already defined. it's also allowing multiple issuers per [brand,petname] composite key.
I see that's unlikely to happen because of how this is being called, but that makes me wonder why this is parameterized. if it were defined below const issuer = it could get the values it needs from the closure. you could even IIFE it without naming the function.
please simplify
There was a problem hiding this comment.
why "introduce" instead of "add"?
The idea was "introduce" as in: grant access to, not just "add something to a data structure".
That is: in the sense of Granovetter diagrams.
I was trying to think thru the flow where users decide which issuers to rely on. But I think this usage might be backwards. Alice the App introduces Bob the user/wallet to Carol the issuer. So the user/wallet is not doing the introducing. Also, Bob doesn't have a choice when Alice introduces Carol to him. And this is all about choosing whether to rely on an issuer.
Does acceptIssuer seem reasonable?
There was a problem hiding this comment.
if it were defined below
const issuer =...
yea, I'm inclined to just inline it there
| * @param {Issuer} issuer | ||
| * @param {Brand} brand | ||
| */ | ||
| const mutualCheck = async (issuer, brand) => { |
There was a problem hiding this comment.
"assert" would make clear that this does nothing but throw. and the verb should come first.
| const mutualCheck = async (issuer, brand) => { | |
| const assertMutual = async (issuer, brand) => { |
| try { | ||
| await mutualCheck(issuer, brand); | ||
| } catch (err) { | ||
| console.warn('issuer/brand mismatch', err); | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
won't the error appear in the console upon throwing?
a failure of mutualCheck seems quite exceptional for a call to providePurseForKnownBrand. consider simplifying this to
| try { | |
| await mutualCheck(issuer, brand); | |
| } catch (err) { | |
| console.warn('issuer/brand mismatch', err); | |
| return undefined; | |
| } | |
| await assertMutual(issuer, brand); |
There was a problem hiding this comment.
This code is called from depositFacet.receive(). So throwing here means dropping a payment on the floor.
Unless we try / catch in receive(). But assigning in a try/catch is ugly.
There was a problem hiding this comment.
from discussion: a match failure isn't so exceptional. Implies the brand isn't "known" (in a sane way) so returning undefined fits the description
| * | ||
| * @param {Brand} brand | ||
| * @param {ERef<NameHub>} known - namehub with brand, issuer branches | ||
| * @returns {Promise<Purse | undefined>} undefined if brand is not well known |
There was a problem hiding this comment.
further down I see that you are concerning this function with "well known" because it takes a remote call that you wouldn't want to have to make before calling this and within the function too.
I think it would help to clarify that in the name, like e.g. providePurseIfKnownBrand. Reading the current name again I see it could be interpreted that way "provide purse for a known brand and undefined otherwise" but the "otherwise" is not hinted at.
| appendToStoredArray(queues, brand, payment); | ||
| // @@NOTE: default 'nat' assetKind is not always right here |
There was a problem hiding this comment.
is this tech debt? what are the implications of 0n for the value of a non-nat Amount?
There was a problem hiding this comment.
oops... @@ meant that I intended to deal with it before marking it rfr.
I wonder if we should throw here. Since we didn't deposit it into a purse, we don't actually have exclusive access to the payment. We're in a race with the caller to see who spends it first.
There was a problem hiding this comment.
from discussion: agree it should throw instead of empty. In general receive can fail. It does fail in this moment, even though it saves the payment to try again later.
| @@ -24,7 +30,10 @@ test.before(async t => { | |||
| space0.produce.bankManager.resolve(bankManager); | |||
| return space0; | |||
| }; | |||
| t.context = await makeDefaultTestContext(t, withBankManager); | |||
| const context = await makeDefaultTestContext(t, withBankManager); | |||
| const bfile = name => new URL(name, import.meta.url).pathname; | |||
There was a problem hiding this comment.
not my recollection. other people will have to maintain this code and import from context when they want to use bfile (and scratch their heads: "why, when this module is not exporting anything?" and then go maybe start another discussion and so on)
… but I'll demote this request to non-blocking
| await signAndBroadcast(harden({ method: 'executeOffer', offer })); | ||
| }, | ||
| }); | ||
| bridgeKit.resolve(uiBridge); |
There was a problem hiding this comment.
why have bridgeKit instead of uiBridge?
There was a problem hiding this comment.
I think I don't understand the question.
bridgeKit is a promise (etc.) for when uiBridge is available.
There was a problem hiding this comment.
Understood. I'm questioning why they have different names. IIRC in the bootstrap we'd have the promise in the space have the name of the thing. The promise is for a uiBridge so I'd expect it to be named that. NBD
| @@ -33,11 +42,14 @@ const bigIntReplacer = (_key, val) => | |||
|
|
|||
| const range = qty => [...Array(qty).keys()]; | |||
|
|
|||
| // We use test.serial() because | |||
| appendToStoredArray(queues, brand, payment); | ||
| // @@NOTE: default 'nat' assetKind is not always right here |
There was a problem hiding this comment.
Does @@NOTE do anything useful?
There was a problem hiding this comment.
it's supposed to remind me to do something about it before marking the PR ready for review
This performance optimization avoids all the work of getting an invitation if there aren't sufficient funds for `proposal.give`. fixes: #7098
WIP: note limitation on depositFacet assetKind
- feat(smart-wallet): purses for well-known brands
- use side table for new purse storage
- issuer/brand mutual check
- storage faciliteates transition
to decentralized issuer introduction
- take care with .init() vs. .set()!
- diagnosis complicated by obscure "no ordinal" diagnostic
- non-vbank asset test tells story in t.log
- test: demonstrate how to share brand displayInfo
- test: spend before receive non-vbank asset
build(smart-wallet): bundle-source devdep
SQUASHME: test: bundle handling, IST_UNIT, ...
SQUASHME: refactor based on review feedback
- XXX move? -> TODO
- mutualCheck -> assertMutual with independent failures
- hoist getBrandToPurses()
- rename to getPurseIfKnownBrand()
- inline addIssuer (aka acceptIssuer)
- throw on mutualCheck failure
SQUASHME: xP
fixup receive throw
fixup getPurseIfKnownBrand
turadg
force-pushed
the
7226-sw-nft-issuer
branch
from
9ffd097 to
0db293c
Compare
dckc
force-pushed
the
7226-sw-nft-issuer
branch
from
0db293c to
c2a8b98
Compare
feat(smart-wallet): trading in well-known non-vbank assets
refs: #7226
Description
Enrich the offer and deposit handling in the smart wallet to support well-known issuers -- that is, issuers in
agoricNames.issuerand correspondinglyagoricNames.brand.Also, demonstrate in a test how permissioned contract deployment would enable trading in non-vbank assets.
Security Considerations
To date, aside from EC member invitations and Oracle operator invitations, the smart wallet contract held no precious assets; assets such as IST and USDC are reflected from cosmos
bankmodule balances via thevbank.This PR adds purses that hold assets that are not recorded anywhere else.
Scaling Considerations
When we make a new purse from a well-known issuer, we do a reverse-lookup by brand. One approach would be to add a method on
NameHubfor that, as discussed in...The current approach here is to use the existing
E(E(agoricNames).lookup('brand')).entries()API and then search thru the entries on the caller's side.Documentation Considerations
The convention for publishing
{ displayInfo }atpublished.boardAux.board0123is only documented in a test. (That'll change in the subsequent upgrade PR).agoricNamesAdminis noted in the discussion of consume under Permisioned Deployment, but the wholeNameHubAPI is more or less undocumented:Testing Considerations
The tests tell a bit of a story using
t.log(). (details below)✔ trading in non-vbank asset: game real-estate NFTs (2.7s)
✔ non-vbank asset: give before deposit
Given that we're introducing purses that store precious assets, would it be cost-effective to test that we know how to trace purse ownership and balance at the kernel database level? #8138
Upgrade Considerations
Upgrading the
walletFactorycontract and to publish displayInfo for IST etc. underboardAuxis to follow in a separate PR.