-
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
more RUN remnants #5913
more RUN remnants #5913
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -209,7 +209,7 @@ export const start = async ( | |
const publicFacet = augmentPublicFacet( | ||
Far('stakeFactory public', { | ||
makeLoanInvitation: () => | ||
zcf.makeInvitation(offerHandler, 'make RUNstake'), | ||
zcf.makeInvitation(offerHandler, 'make stakeFactory'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. This change is part of the visible contract API, so it's more important than many of the others. Please consider separating it out as a separate commit marked as a breaking change fix. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Visible to end users but not read by machines, correct? I.e. I don't believe this change breaks any commitments of the API. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would make sense for a machine to select among invitations from an installation based on description. This is discussed in #2230 for example:
|
||
makeReturnAttInvitation: att.publicFacet.makeReturnAttInvitation, | ||
}), | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,7 +56,7 @@ const calculateFee = (feeCoeff, currentDebt, giveAmount, wantAmount) => { | |
*/ | ||
|
||
/** | ||
* Make RUNstake kit state | ||
* Make stakeFactory kit state | ||
* | ||
* @param {ZCF} zcf | ||
* @param {ZCFSeat} startSeat | ||
|
@@ -358,7 +358,7 @@ const helperBehavior = { | |
helper.assertVaultHoldsNoMinted(); | ||
seat.exit(); | ||
|
||
return 'Your RUNstake is closed; thank you for your business.'; | ||
return 'Your stakeFactory is closed; thank you for your business.'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Messages like this don't play a critical security role like invitation descriptions, but strictly speaking, this is also a breaking contract API change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The API just says that it returns a string. Anything depending on parsing that string is making unwarranted assumptions about the API. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have quite a few tests that depend on the contents of these strings. That seemed like an unwarranted assumption to me too, but that seems to be the current design. I guess it doesn't matter that much... |
||
}, | ||
}; | ||
|
||
|
@@ -418,15 +418,15 @@ const finish = ({ facets }) => { | |
}; | ||
|
||
/** | ||
* Make RUNstake kit, subject to stakeFactory terms. | ||
* Make stakeFactory kit, subject to stakeFactory terms. | ||
* | ||
* @param {ZCF} zcf | ||
* @param {ZCFSeat} startSeat | ||
* @param {import('./stakeFactoryManager.js').StakeFactoryManager} manager | ||
* @throws {Error} if startSeat proposal is not consistent with governance parameters in manager | ||
*/ | ||
export const makeStakeFactoryKit = defineKindMulti( | ||
'RUNStakeKit', | ||
'stakeFactoryKit', | ||
initState, | ||
behavior, | ||
{ finish }, | ||
|
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.
things like this really should be looked up from a namehub such as
E(home).agoricNames
(see also #4819), so adding features toconfigureVaultFactoryUI
seems like the wrong direction. Not critical, I suppose.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.
Yeah. Easy enough to remove when that work is done. Meanwhile I don't want to slow down the dapps moving off RUN_*