-
Notifications
You must be signed in to change notification settings - Fork 212
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
provide starter IST to newly provisoned accounts #6157
Conversation
17d0c8a
to
560b676
Compare
how to authorize a module account to receive funds?
p.s. Answer: don't disable it |
a3da596
to
9c3208e
Compare
First end-to-end test worked!
Send USDC to the pool account
provisionPool trades the USDC for IST
Provision a smart-wallet accountNOTE: This will cost N BLD shortly.
provisionPool provides a smartWallet and sends 0.25 ISTNOTE special coordination around essential purses, i.e. IST.
Tada! money in my pocket
cc @JimLarson @dtribble @turadg @michaelfig @rowgraus @samsiegart @Tartuffo |
9c3208e
to
808ccae
Compare
70dc61e
to
5fca22c
Compare
// @ts-expect-error vbank purse is close enough for our use. | ||
const fundPurse = E(poolBank).getPurse(poolBrand); |
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.
did you run accross this, @turadg ? any clues?
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.
yes. VirtualPurse is what the bank returns and it's slightly different. For example, withdraw
is sync on Purse and async on VirtualPurse. @michaelfig documented this,
agoric-sdk/packages/vats/test/test-vat-bank.js
Lines 135 to 142 in 28b104b
// Withdrawal. We can't easily type a `VirtualPurse` unless we make it | |
// callable only with `E`, in which case we can't automatically unwrap the | |
// return result of `E(vpurse).withdraw` to a sync interface (which `Payment` | |
// is). | |
// | |
// TODO: We can fix this only if the ERTP methods also allow consuming a | |
// `Remote<Payment>` instead of just `Payment`. That typing has not yet been | |
// done, hence the cast. |
Making ERTP methods accept remote payments seems worth doing soon. WDYT @michaelfig ?
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.
Golang changes look good!
@@ -2,6 +2,7 @@ import bundleCentralSupply from '@agoric/vats/bundles/bundle-centralSupply.js'; | |||
import bundleMintHolder from '@agoric/vats/bundles/bundle-mintHolder.js'; | |||
import bundleSingleWallet from '@agoric/vats/bundles/bundle-singleWallet.js'; | |||
import bundleWalletFactory from '@agoric/vats/bundles/bundle-legacy-walletFactory.js'; | |||
import bundleProvisionPool from '@agoric/vats/bundles/bundle-provisionPool.js'; |
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'd like to stop maintaining packages/wallet/contract
. @samsiegart @michaelfig do we need it any more?
@turadg bonus points if you beat me to it.
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.
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.
Cursory review from phone. Only blocker I see is the Kit name. I'll look into types when not afk but can merge as is imo.
packages/vats/src/provisionPool.js
Outdated
console.info('PLEASE_PROVISION', address, powerFlags); | ||
|
||
if (!powerFlags.includes(PowerFlags.SMART_WALLET)) { | ||
return; |
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.
Why not error? I haven't looked closely to answer for myself, but would be good yo have an explanation comment here regardless.
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.
Not sure. I have a vague memory that I tried that and something bad happened. But I should look 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.
Now I remember: I was thinking we could register 2 handlers for BRIDGE_ID.WALLET
, on for SMART_WALLET
and one for REMOTE_WALLET
. But now that I think about it, we can only register one handler.
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.
This looks basically right. I suggested some clean-ups.
9bff42b
to
19288e3
Compare
oops... @JimLarson I didn't mean to re-request a review from you. wrong button |
@@ -15,35 +14,67 @@ const { details: X } = assert; | |||
// eslint-disable-next-line no-unused-vars | |||
const startFactoryInstance = (zoe, inst) => E(zoe).startInstance(inst); | |||
|
|||
const StableUnit = BigInt(10 ** Stable.displayInfo.decimalPlaces); |
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.
wonder if units should go in with their Brand def (i.e. tokens.js in this case)
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.
Perhaps, if there are other static uses of it... but a quick search doesn't find any other occurrences of 10 ** Stable.displayInfo.decimalPlaces
.
console.warn('startWalletFactory needs bridgeManager (not sim chain)'); | ||
return; | ||
} | ||
console.log('provision pool', { poolAddr }); |
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.
Should this be dropped or turned into trace()
?
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.
as much as any other logging we do, I suppose; clean-up is postponed to #5222 , IIUC.
I'm only removing my request for changes, not actually adding an explicit approval. Not intending to block submission. I didn't see another way to stop blocking than to approve, so that's the button I pushed. |
1ed39c7
to
1b329dd
Compare
- vbank/provision, which is _not_ blocked from receiving funds
- build bundle-provisionPool
- integrate provisionPool into startWalletFactory - don't try to sell IST for IST - makeHandler takes a record of named args (...hunt) - clean up moduleAccountAddress stuff - docs: static type for provision pool facets
using labels in patterns
- never mind PROVISION_ACCT_TEST
1b329dd
to
f164232
Compare
refs: #6067
Description
provisionPool
contract; wire it up to the walletFactory as well as each PSMvbank/provision
, to hold IST as well as assets to sell on the/a PSMTODO:
Security Considerations
In the contract:
makeBridgeProvisionTool
is factored out of the contractstart
function to keep the bridge handling stuff mostly separate from the asset-handling stuff.Elsewhere, the use of bootstrap powers merits careful review.
Documentation Considerations
We should provide end users with docs on the "pay 10 BLD to the community pool; get 0.25 IST for initial fees" model.
Testing Considerations
Trading on the PSM is unit-tested.
The flow resulting in 0.25 IST going to the user was only tested manually. (Results are documented below.)