refactor(run-protocol): extract a vaultKit module #4743
Conversation
turadg
force-pushed
the
ta/extract-vaultKit
branch
from
6e754df to
e2c6d67
Compare
| @@ -105,6 +77,28 @@ const makeOuterKit = inner => { | |||
| * @property {(oldDebt: Amount, oldCollateral: Amount, vaultId: VaultId) => void} updateVaultPriority | |||
| */ | |||
|
|
|||
| /** | |||
| * @typedef {Readonly<{ | |||
There was a problem hiding this comment.
I put these in here but stopped short of using them because they're available in closure scope and it doesn't make much sense to change them before factoring out makeMethods or whatever we call it. I may have time for that before Monday if it's worthwhile, though I'd prefer to land the non-state parts of this PR first.
| makeAdjustBalancesInvitation: () => | ||
| owned(vault).makeAdjustBalancesInvitation(), | ||
| makeCloseInvitation: () => owned(vault).makeCloseInvitation(), | ||
| makeTransferInvitation: () => { |
There was a problem hiding this comment.
Adding a comment here would be good: "Starting a transfer revokes the outer vault. The associated updater will get a special notification the the vault is being transferred."
| }; | ||
|
|
||
| /** | ||
| * |
There was a problem hiding this comment.
Return the kit for a vault in the structure that wallets are expecting
There was a problem hiding this comment.
Let me know if you're talking about more than uiNotifier discussed in #4743 (comment) .
| export const makeVaultKit = (inner, assetNotifier) => { | ||
| const { vault, vaultUpdater } = wrapVault(inner); | ||
| const vaultKit = harden({ | ||
| assetNotifier, |
There was a problem hiding this comment.
The structure here is not what wallets are expecting. They need uiNotifier. This got renamed, but it means that the wallet doesn't know to treat it specially (in lib-wallet getUINotifier).
There was a problem hiding this comment.
The vault notifications were broken by #4527 because they can no longer rely on one notifier. We discussed multiple designs in the RUN protocol meeting and agreed this was the best option. 554d41e renamed uiNotifier to make it clear to consumers that there was no longer a single notifier for UI.
I think that's the right thing for now for clarity of how the contract works. ("UI updater" is a misnomer.) Updating the UI code is queued up in #4482 which can happen after Restival because UI is not in scope.
| * runMint: ZCFMint, | ||
| * vaultSeat: ZCFSeat, | ||
| * zcf: ContractFacet, | ||
| * }>} ImmutableState |
There was a problem hiding this comment.
I wonder if we should have props a la React for immutable and have separate storage for those!
There was a problem hiding this comment.
Neat idea. We'd need the virtual objects API to send them in. Could be useful for distinguishing between things the contract gets to change vs things it just needs to be provided consistently.
dtribble
force-pushed
the
ta/extract-vaultKit
branch
from
33c03fb to
13bdd71
Compare
turadg
force-pushed
the
ta/extract-vaultKit
branch
from
13bdd71 to
06103d8
Compare
turadg
added
the
automerge:no-update
Description
Several refactorings and cleanups. I split across multiple commits to see the flow and I'll autosquash upon approval.
Security Considerations
None.
Documentation Considerations
No, implementation details.
Testing Considerations
No changes.