refactor(run-protocol): extract `chargeInterest` for use outside vaults #4732

turadg · 2022-03-03T22:42:18Z

Description

getRUN will be charging interest like vaults do, so this pulls a chargeInterest function out. It has the added benefit of generalizing interest charging to any kind of NatValue brand.

Moves interest utils up to root of run-protocol since they're for more than just vaults now. Also does some validation that the brands match up.

Will merge as squash.

Security Considerations

No change in powers.

Documentation Considerations

--

Testing Considerations

CI sufficient.

Chris-Hibbert

I like the refactoring and relocation of interest calculation, though there's an associated piece that should be moved out of Zoe, I think.

On the changes to type declaration of Amounts, I'll have to defer to @erights

You have my LGTM once MarkM approves the ERTP type changes.

Chris-Hibbert · 2022-03-03T23:39:04Z

packages/ERTP/src/issuerKit.js

+ * from the mathHelpers library. For example, natMathHelpers, the
+ * default, is used for basic fungible tokens.
+ *
+ *  `displayInfo` gives information to UI on how to display the amount.


Suggested change

* `displayInfo` gives information to UI on how to display the amount.

* `displayInfo` gives information to the UI on how to display the amount.

I realize you only moved these comments, but since I spotted typos, now seems the right time to fix them.

Chris-Hibbert · 2022-03-03T23:39:44Z

packages/ERTP/src/issuerKit.js

+ * @param {ShutdownWithFailure=} optShutdownWithFailure If this issuer fails
+ * in the middle of an atomic action (which btw should never happen), it
+ * potentially leaves its ledger in a corrupted state. If this function was
+ * provided, then it the failed atomic action will call it, so that some


Suggested change

* provided, then it the failed atomic action will call it, so that some

* provided, then the failed atomic action will call it, so that some

Chris-Hibbert · 2022-03-03T23:48:06Z

packages/run-protocol/src/vaultFactory/prioritizedVaults.js

+ * @param {Amount<'nat'>} debtAmount
+ * @param {Amount<'nat'>} collateralAmount


Is there any chance we can make a constant or symbol to use here rather than a bare string? I realize this is a type declaration rather than javascript, so the answer may be "no", which would be unfortunate. If it only had to appear in our code that would be one thing, but if authors of contracts have to do this, I'm concerned about the opportunity for typos.

Yes, tsc should grok a const NAT = 'nat'; thing, I'm told (and I've seen).

Whether it's smart enough for const { NAT } = AssetKind;, I don't know.

concerned about the opportunity for typos

The type parameter K must be a subtype of AssetKind which is the union of 'nat' | 'set' | 'copySet' | 'copyBag', so if someone were to type any string other than one of those four it would be a type failure. (type)

There may be an aesthetic argument to use AssetKind.NAT (const) but it would be more verbose without any safety benefit: Amount<AssetKind.NAT>.

Dan's Amount<NAT> idea would b no more verbose and a little cleaner but then you'd have to define or import NAT in the file you're typing that. (That import works, btw, because AssetKind object's type is a not [index]: string but literally each key to each value.)

Given how easy this would be to revert or change to either of the above, can we let it through and adjust as needed?

Second thought, this PR is unblocking and the types are just nice to have so I extracted to #4736

Given how easy this would be to revert or change to either of the above, can we let it through and adjust as needed?

I don't feel comfortable speaking for @erights on that.

Chris-Hibbert · 2022-03-04T00:00:54Z

packages/zoe/src/contractSupport/interest.js

@@ -27,10 +27,10 @@ const calculateRelativeCompounding = (

 /**
 *
- * @param {Amount<NatValue>} debtSnapshot
+ * @param {Amount<'nat'>} debtSnapshot


All the methods in this file are only used within Vaults. Let's move it there for a start.

The follow-on question is whether it should be merged in with the new interest.js. I guess these only apply to the way that Vaults aggregate interest charges at the VaultManager level, so that would argue that it makes sense to leave them separate.

Would you mind moving it and adding comments about how special-purpose this is?

…sed by getRUN

turadg · 2022-03-04T15:05:33Z

On the changes to type declaration of Amounts, I'll have to defer to @erights. You have my LGTM once MarkM approves the ERTP type changes.

Since that change is tangential to supporting getRUN I've pulled it off to #4736

turadg · 2022-03-04T17:57:59Z

call it keyword rather than name.

This makes sense, will do.

I think we never use alleged names for anything other than debugging / display. I suggest making this an argument to chargeInterest.

Oh I wish this before I ran into the async lookup problem that required 27116fc. Is there no way to find out from a mint what keyword it expects in its mintGains method? I'll look in getIssuerRecord

dckc · 2022-03-04T18:50:30Z

... Is there no way to find out from a mint what keyword it expects in its mintGains method?

There's a misunderstand somewhere around here. Mints don't expect keywords. Zoe and contracts do. Keywords are part of the Zoe API, not the ERTP API.

turadg · 2022-03-04T19:07:11Z

Mints don't expect keywords

Maybe we're talking about different keywords? I'm referring to the AmountKeywordRecord type that mintGains of the mint expects.

Chris-Hibbert · 2022-03-04T19:30:56Z

the AmountKeywordRecord type that mintGains of the mint expects.

The mint/issuer doesn't constrain the type used there. That's a convention that the contract uses to coordinate between the seats. If the parts of your contract agree, you could call it Gains: or Foo:, and everyone would be happy.

dckc · 2022-03-04T21:44:32Z

@Chris-Hibbert writes:

You have my LGTM once MarkM approves the ERTP type changes.

and @turadg took the commit with those changes out. So I suppose we're ready to go here.

turadg requested review from dckc and Chris-Hibbert March 3, 2022 22:42

turadg force-pushed the ta/getRun-support branch from 67fe9df to 7e1e932 Compare March 4, 2022 00:05

Chris-Hibbert reviewed Mar 4, 2022

View reviewed changes

turadg added 2 commits March 4, 2022 06:52

refactor(run-protocol): extract chargeInterest()

7eeabcc

refactor(run-protocol): move interest.js up from vaultFactory to be u…

43255eb

…sed by getRUN

turadg force-pushed the ta/getRun-support branch from 7e1e932 to 43255eb Compare March 4, 2022 14:52

turadg mentioned this pull request Mar 4, 2022

chore: improve use of AssetKind type #4736

Merged

turadg changed the title ~~refactoring to support getRUN feature~~ refactor(run-protocol): extract chargeInterest for use outside vaults Mar 4, 2022

turadg requested a review from Chris-Hibbert March 4, 2022 15:06

turadg marked this pull request as ready for review March 4, 2022 15:06

turadg added the automerge:squash Automatically squash merge label Mar 4, 2022

fix: getting brand name

505a3f5

turadg marked this pull request as draft March 4, 2022 15:54

test(run-protocol): handle async chargeInterest

27116fc

chore: cleanup

0ea6924

turadg added 2 commits March 4, 2022 12:06

fix: pass in seatAllocationKeyword

776e0b3

fix: type for makeIssuerRecord

b610673

turadg marked this pull request as ready for review March 4, 2022 20:18

dckc approved these changes Mar 4, 2022

View reviewed changes

mergify bot merged commit 0c1a686 into master Mar 4, 2022

mergify bot deleted the ta/getRun-support branch March 4, 2022 21:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(run-protocol): extract `chargeInterest` for use outside vaults #4732

refactor(run-protocol): extract `chargeInterest` for use outside vaults #4732

turadg commented Mar 3, 2022 •

edited

Loading

Chris-Hibbert left a comment

Chris-Hibbert Mar 3, 2022

Chris-Hibbert Mar 3, 2022

Chris-Hibbert Mar 3, 2022

dckc Mar 4, 2022

turadg Mar 4, 2022

turadg Mar 4, 2022

dckc Mar 4, 2022

Chris-Hibbert Mar 4, 2022

turadg commented Mar 4, 2022

turadg commented Mar 4, 2022

dckc commented Mar 4, 2022

turadg commented Mar 4, 2022

Chris-Hibbert commented Mar 4, 2022

dckc commented Mar 4, 2022

	* `displayInfo` gives information to UI on how to display the amount.
	* `displayInfo` gives information to the UI on how to display the amount.

	* provided, then it the failed atomic action will call it, so that some
	* provided, then the failed atomic action will call it, so that some

		* @param {Amount<'nat'>} debtAmount
		* @param {Amount<'nat'>} collateralAmount

refactor(run-protocol): extract chargeInterest for use outside vaults #4732

refactor(run-protocol): extract chargeInterest for use outside vaults #4732

Conversation

turadg commented Mar 3, 2022 • edited Loading

Description

Security Considerations

Documentation Considerations

Testing Considerations

Chris-Hibbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg commented Mar 4, 2022

turadg commented Mar 4, 2022

dckc commented Mar 4, 2022

turadg commented Mar 4, 2022

Chris-Hibbert commented Mar 4, 2022

dckc commented Mar 4, 2022

refactor(run-protocol): extract `chargeInterest` for use outside vaults #4732

refactor(run-protocol): extract `chargeInterest` for use outside vaults #4732

turadg commented Mar 3, 2022 •

edited

Loading