-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: swap contract #10
Conversation
c7e72f9
to
3d6685e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me.
a2f3f1a
to
aa79dc3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments and questions. All non blocking.
contract/src/objectTools.js
Outdated
/** @type { <T extends Record<string, ERef<any>>>(obj: T) => Promise<{ [K in keyof T]: Awaited<T[K]>}> } */ | ||
export const allValues = async obj => { | ||
const es = await Promise.all( | ||
entries(obj).map(([k, vp]) => Promise.resolve(vp).then(v => [k, v])), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merely a suggestion:
entries(obj).map(([k, vp]) => Promise.resolve(vp).then(v => [k, v])), | |
entries(obj).map(([k, vp]) => E.when(vp, v => [k, v])), |
}); | ||
await E(addressAdmin).default(DEPOSIT_FACET_KEY, depositFacet); | ||
|
||
const getContractInvitation = invitationSpec => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[style]: would it be helpful to declare a type for invitationSpec
?
contract/test/wallet-tools.js
Outdated
}; | ||
|
||
const getPurseInvitation = async invitationSpec => { | ||
// const { instance, description } = invitationSpec; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this line obsolete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's sort of part of the todo... which I have just todone
// XXX throwing here is unhandled somehow. | ||
const seat = await E(zoe).offer(invitation, proposal, pmts, offerArgs); | ||
seatById.set(offerSpec.id, seat); | ||
// console.log(address, offerSpec.id, 'got seat'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trace()
is useful for log messages you don't want to lose.
contract/test/test-swap-wallet.js
Outdated
|
||
const { bundleCache } = t.context; | ||
const bundle = await bundleCache.load(assets.swaparoo, contractName); | ||
const bundleID = `b1-${bundle.endoZipBase64Sha512}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just saw a helper for this, didn't I?
|
||
const cowAmount = AmountMath.make( | ||
wellKnown.brand.BLD, | ||
// makeCopyBag([['Milky White', 1n]]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[non-blocking]: is something broken with non-fungibles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-fungibles (at least: that particular one) aren't conveniently available in an end-to-end testing context (#16)
As I mentioned, I keep moving bits of stuff up and down my stack of PRs... I should think about this one a bit more...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some non-blocking comments after reading through the contract, I'm reading the tests next.
contract/src/swaparoo.contract.js
Outdated
}); | ||
}; | ||
|
||
const { want, give } = firstSeat.getProposal(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick
Rename these to { firstSeatWant, firstSeatGive }
so it's a little bit easier to read?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that added a bunch of line wrapping. { want: want1, give: give1 }
seems like a happy medium
- prune dead code - avoid ++ - fix shadow
- prune dead code - add static types
- one-line await is too clever
- factor our bundle-tools - prune boardAux hack - move type burden inside contract starters' - initialize balances - punt chainStorage until we use CapData across vats - chore(boot-tools): produce BLD, startUpgradable
- chore: notifier -> async iterator - seatLike: return on payout amounts - missing param decl; lint - prune unused makeWalletFactory - getPayouts -> getPayoutAmounts, with type - docs(wallet-tools): invitationSpec types
- OfferStatus -> offerStatus - use seatLike - getPayoutAmounts - clean up TODO
- refactor: use wallet-tools - refactor: follow x.contract.js pattern - refactor: use getBundleId helper
- refactor: clarify want1, give1 - style: lint
refs #9
This meets all the requirements I know of (oops; save 1... deploying on chain):
from Agoric/documentation#940 :
makeInvitation
calls has an offer shape; is that enough?It's a minimal effort job. I copied
test-swap-wallet.js
from dapp-swaparoo, ran it, and then fixed the errors until it passed. In all cases, copying verbatim from dapp-swaparoo sufficed.Reviewers: please distinguish correctness feedback from style. Here's hoping we find time to make the code exemplary in style as well as correct, but I'm not sure how much of that we should do at this point.