feat(run-protocol): RUNstake contract only, without payoff from rewards #4741
Conversation
dckc
force-pushed
the
dc-getRUN-contract
branch
4 times, most recently
from
13992d2 to
586e16f
Compare
|
@Chris-Hibbert @dtribble @turadg What do you think about landing this much; that is; without interest, and without integration with the front end and bootstrap? |
|
What made me think of landing this much is: it basically handles the cases that @rowgraus and @JimLarson and I worked out in a spreadsheet in December: I'm going to take a break and think it over... |
| * @returns {Promise<Payment>} | ||
| */ | ||
| const mintZCFMintPayment = (zcf, zcfMint, amountToMint) => { |
There was a problem hiding this comment.
why does addReturnableLien want to return a payment directly, rather than adding an allocation to a seat so Zoe can manage offer safety and invitations and such?
There was a problem hiding this comment.
I don't know. It could be left over from when there were to kinds of attestations. I got rid of the timed attestation code and tested that the remaining stuff passes its tests but I haven't gotten as far as I'd like getting down to only the essential code.
There was a problem hiding this comment.
I figured it out and added a comment:
* To support `makeAttestation`, we directly mint attestation payments.
* We still use a ZCFMint because returning attestations is done
* through Zoe offers.
016514c to
3c522e0
Compare
dckc
changed the title
dckc
force-pushed
the
dc-getRUN-contract
branch
7 times, most recently
from
6baba27 to
aadbc35
Compare
| * | ||
| * @type {<K, V>(store: Store<K, V>, key: K, thunk: () => V) => V} | ||
| */ | ||
| const getOrElse = (store, key, make) => { |
There was a problem hiding this comment.
is this something Store itself will support @FUDCo ?
There was a problem hiding this comment.
I confess I don't understand what you are asking.
There was a problem hiding this comment.
This PR defines a getOrElse function that lets the caller say "give me the value for this key from the store, or if it's not there define it in the store". That seems generally useful so I asked if the Store API might offer that in the future. If not the MapStore type then perhaps a helper like this within @agoric/store ?
There was a problem hiding this comment.
I don't think there are any plans for anything like this. The authority to consult would be @erights, who I tentatively predict, on the basis of no knowledge whatsoever on my part, will have a subtle and nuanced argument why doing this is would be a bad idea.
There was a problem hiding this comment.
@dtribble I remember you had a good verb to mean something like this getOrElse. Do you remember what it was?
@FUDCo I will try to avoid taking that as a challenge ;) .
I would be fine with a helper. I am reluctant to extend the type itself, but I do not currently have any principled objection. Just a general sense that the object's own API should remain reasonably small and any addition must pay for itself.
Whether helper or method, should the make function take the key as a parameter?
There was a problem hiding this comment.
should the
makefunction take the key as a parameter?
Interesting idea. I can see some utility. The design here is inspired by ELib, where fetch/2 took a Thunk, which takes no args.
For reference, monte adopted the same design:
https://monte.readthedocs.io/en/latest/runtime.html#safeScope.Map.fetch
https://github.com/monte-language/typhon/blob/master/typhon/objects/collections/maps.py#L138
| @@ -0,0 +1,55 @@ | |||
| // The proposal is not allowed to include any keys other than these, | |||
There was a problem hiding this comment.
looks like this was meant to be part of a doc comment for assertOnlyKeys
| * @param {Amount} fromGains | ||
| * @param {Keyword} key | ||
| */ | ||
| export const transfer = (fromSeat, toSeat, fromLoses, fromGains, key) => { |
There was a problem hiding this comment.
+1 to pulling this out in a shared module. Heads up there will be some merge conflicts after #4877 (e.g. transfer -> stageDelta atm)
| @@ -442,3 +443,163 @@ export const startRewardDistributor = async ({ | |||
| ); | |||
| }; | |||
| harden(startRewardDistributor); | |||
|
|
|||
| /** | |||
| * @typedef { import('@endo/eventual-send/').Unpromise<T> } Unpromise<T> | |||
There was a problem hiding this comment.
Unpromise is deprecated (in agoric-sdk before those changes were globbered). Instead use Awaited built into TS
| // TODO: remove publicFacet wart | ||
| const attestBrand = await E(att.publicFacet).getBrand(); | ||
|
|
||
| const paramManager = await makeRunStakeParamManager( |
There was a problem hiding this comment.
#4868 will make it so you don't have to hold the full paramManager or check that electorate matches.
| * | ||
| * @type {ReallocateWithFee} | ||
| */ | ||
| const reallocateWithFee = (fee, fromSeat, otherSeat = undefined) => { |
There was a problem hiding this comment.
There was a problem hiding this comment.
my eyes are getting blurry. I'm struggling to see the motivation or something.
There was a problem hiding this comment.
sorry, I considered being more specific and should have been. The link shows an in-flight PR where reallocateWithFee has been expanded in scope to mintAndReallocate. I thought you might want to consider the same for parity.
There was a problem hiding this comment.
Ugh. I'm struggling to get this change right.
| * | ||
| * @param {ZCF} zcf | ||
| * @param {ZCFSeat} startSeat | ||
| * @param {ReturnType<import('./runStakeManager.js').makeRunStakeManager>} manager |
There was a problem hiding this comment.
consider defining RunStakeManager in its file and importing that.
this may become standard practice for makeFoo functions.
| const asset = async ref => | ||
| new URL(await metaResolve(ref, import.meta.url)).pathname; | ||
|
|
||
| t.log('bundling...', contractRoots); |
There was a problem hiding this comment.
How I wish we could cache bundling builds.
|
Dan asked
I don't mind omitting frontend integration from first merge. I have found integration with bootstrap to be informative, and based on conversations with you, I've attempted to make test setup consistent with it, which has made that integration valuable to do early. Interest is straightforward. But parameter governance and interest charging and other library support have been evolving quickly. Perhaps merging early eases migration support enough to be worth the added pain. If it's not ready for use in the economy, the obvious step is to not activate in bootstrap. |
- makeAttestationIssuerKit, makeAttestationFacets - await attestation payment before commit
- makeAttestationFacets: clean up type details
via mintZCFMintPayment
- rename zcfMint -> attMint
- MakeZCFMint is missing? add type for getIssuerRecord return
- keep getLiened after all, since it's used so much in tests,
but don't expose via runStake creatorFacet nor AttestationMaker
Co-authored-by: Turadg Aleahmad <[email protected]>
dckc
force-pushed
the
dc-getRUN-contract
branch
from
62e861c to
349f241
Compare
dckc
changed the title
refs: #3788
Description
getRUN contract only, without rewards integration (#4244) and without integration with bootstrap, front end / wallet, etc.
Security Considerations
We plan to grant this contract access to mint RUN.
Documentation Considerations
MintingRatio, document plausible range of values and risks associated with a setting that is too high or too low.Performance Considerations
checkAccountState(address, lienDelta)given performance implications noted by @JimLarsonTesting Considerations