chore(run-protocol): improve comments and naming #4764
Conversation
turadg
force-pushed
the
ta/fixup-vault-state-type
branch
from
6f7b043
to
b484af5
Compare
turadg
changed the title
Tartuffo
added
audit-restival
turadg
force-pushed
the
ta/fixup-vault-state-type
branch
from
2245d4e
to
69b3962
Compare
| @@ -142,8 +144,8 @@ export const makeInnerVault = ( | |||
|
|
|||
| // Two values from the same moment | |||
| interestSnapshot: manager.getCompoundedInterest(), | |||
| // @ts-expect-error until https://github.com/Agoric/agoric-sdk/pull/4736 | |||
| runSnapshot: AmountMath.makeEmpty(runBrand), | |||
| /** @type {any} cast */ | |||
There was a problem hiding this comment.
This contract only handles fungible debt, right? Have you tried declaring the debtBrand about (around line 120) to Brand<'nat'> to see if this cast becomes unnecessary? I wonder if getIssuerRecord gives you the assetKind so that you could do a runtime check to be sure.
There was a problem hiding this comment.
Brand is not parameterized yet in master. #4736 will fix this and the expect-error will error to be cleaned up.
| // XXX 'run' is implied by the brand in the amount | ||
| debtSnapshot: { run, interest }, |
There was a problem hiding this comment.
perhaps rename things to { amount, interest }?
There was a problem hiding this comment.
naming and organization to be cleaned up in #4482
| runMint.mintGains(harden({ RUN: toMint }), vaultSeat); | ||
| mint.mintGains(harden({ RUN: toMint }), vaultSeat); |
There was a problem hiding this comment.
not ready to change the RUN keyword yet?
There was a problem hiding this comment.
No that's queued up in #4800 and blocked on a design decision for what it will be.
| /** | ||
| * Public API of the vault. | ||
| * | ||
| * @see {InnerVault} for the internal API it wraps. |
There was a problem hiding this comment.
That's perhaps expedient, but for actual clients of this contract, referring them to the inner guts doesn't seem like the way to go.
mostly thinking out loud... I suppose it's more relevant for agoric.com/documentation , but I have hopes of generating that from this.
There was a problem hiding this comment.
I agree and see that as out of scope. We need a bigger discussion around naming of contract facets.
There was a problem hiding this comment.
I'm not talking about the name of the facets. I'm talking about information hiding, or lack thereof. Clients of this API should not need to be aware of the inner API.
There was a problem hiding this comment.
Ohhh. Just this jsdoc @see.
I'm not sure. Developers of agoric-sdk should know about it. End users of a GUI or REPL shouldn't (and won't see the jsdoc). What other case is there? Developers building other contracts that compose the vaultFactory contract? In that case I think it behooves them to understand the inner/outer relationship (e.g. to understand how transfers work).
| runMint.burnLosses, | ||
| debtMint.burnLosses, |
There was a problem hiding this comment.
debtMint is a bit nicer than bare mint.
* chore(run-protocol): improve comments and naming * refactor(run-protocol): use key encoding from @agoric/store * refreshLoanTracking -> updateDebtAccounting Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Description
ts-expect-errorwhich revealed an error in refactor(run-protocol): extract a vaultKit module #4743anyso it can cast to the type withinstatewithout suppression. It then fixes type errors that were detected.Security Considerations
--
Documentation Considerations
Gotcha documented in https://github.com/Agoric/agoric-sdk/wiki/Typescript-using-JSDoc/
Testing Considerations
--