Skip to content
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.

Sign up for GitHub

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

Jump to bottom

refactor(run-protocol)!: remove interestRate and liquidationRatio from vault notifier #5009

Merged
merged 3 commits into from
Jul 7, 2022
Merged

refactor(run-protocol)!: remove interestRate and liquidationRatio from vault notifier #5009

merged 3 commits into from
Jul 7, 2022

Conversation

turadg
Copy link
Member

@turadg turadg commented Apr 5, 2022 •
edited
Loading

closes: #4685

Description

Neither were used anywhere.

When needed, interestRate is now on AssetNotifier.

Security Considerations

--

Documentation Considerations

Breaking change, denoted in conventionalcommit syntax.

Testing Considerations

@turadg turadg requested a review from samsiegart April 5, 2022 16:01
@turadg turadg linked an issue Apr 5, 2022 that may be closed by this pull request
remove vault manager notifications from vault's notifications #4685
Closed
@turadg turadg changed the title refactor(run-protocol)!: remove interestRate and liquidationRatio from vault notifier" refactor(run-protocol)!: remove interestRate and liquidationRatio from vault notifier Apr 5, 2022
dtribble
dtribble reviewed Apr 5, 2022
View reviewed changes
packages/run-protocol/test/vaultFactory/test-vaultFactory.js
@@ -969,11 +969,6 @@ test('interest on multiple vaults', async t => {
),
AmountMath.make(runBrand, 3200n + bobAddedDebt),
);
t.deepEqual(bobUpdate.value.interestRate, rates.interestRate);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these getting tested in the vaultManager's notifier?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, nothing in agoric-sdk is reading these removed values. I'll start some new tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Dean is asking a different question. These values are now superfluous in vault.js because all the relevant work is done in vaultManager.js. I checked, and the values are visible to the UI from the versions that would remain in vaultManager.js. The question is whether there are tests corresponding to the one removed here that includes verifying the new state of the values in vaultManager's updates.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question is whether there are tests corresponding to the one removed here that includes verifying the new state of the values in vaultManager's updates.

Yep, that was my understanding. The answer to that question is "no" and that's why I'm going to "start some new tests".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question is whether there are tests corresponding to the one removed here that includes verifying the new state of the values in vaultManager's updates.

I said "no" but that was wrong. Here the assetNotifier from vaultManager is read: https://github.com/Agoric/agoric-sdk/blame/540978c997e30c25905ea1fb837ae701db6f3536/packages/run-protocol/test/swingsetTests/governance/vat-alice.js#L47-L63

@samsiegart
Copy link
Contributor

The UI uses both of these currently.

When needed, interestRate is now on AssetNotifier.

Sounds good.

How do we show the liquidation ratio now? I suppose we can get that from getCollaterals() and check from the vault's collateral. Can the liquidation ratio and interest rate change after you've opened a vault/could they be different between new vaults and old vaults of the same collateral type?

Chris-Hibbert
Chris-Hibbert approved these changes Apr 5, 2022
View reviewed changes
Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

The question about whether the corresponding update is verified in vaultManager should be answered here, but if the answer is no, the fix can come later. (so I guess I'm asking for either a fix here or an issue #.)

packages/run-protocol/test/vaultFactory/test-vaultFactory.js
@@ -969,11 +969,6 @@ test('interest on multiple vaults', async t => {
),
AmountMath.make(runBrand, 3200n + bobAddedDebt),
);
t.deepEqual(bobUpdate.value.interestRate, rates.interestRate);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Dean is asking a different question. These values are now superfluous in vault.js because all the relevant work is done in vaultManager.js. I checked, and the values are visible to the UI from the versions that would remain in vaultManager.js. The question is whether there are tests corresponding to the one removed here that includes verifying the new state of the values in vaultManager's updates.

@turadg
Copy link
Member Author

turadg commented Jun 30, 2022 •
edited
Loading

Next steps:

  • drop the need for integration tests, not happening for MN-1
  • don't move interestRate and liquidationRatio to the asset notifier since they're already exposed as governed params. consumers can merge subscriptions if they need a single stream.
  • move liquidatorInstance to governance

@turadg turadg force-pushed the 4685-remove-asset-notifications-from-vault branch from f9bf3b4 to be11fa8 Compare July 7, 2022 20:30
@turadg turadg added the automerge:rebase Automatically rebase updates, then merge label Jul 7, 2022
@mergify mergify bot merged commit 6d6b560 into master Jul 7, 2022
@mergify mergify bot deleted the 4685-remove-asset-notifications-from-vault branch July 7, 2022 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtribble dtribble dtribble left review comments

@Chris-Hibbert Chris-Hibbert Chris-Hibbert approved these changes

@samsiegart samsiegart Awaiting requested review from samsiegart

Assignees
No one assigned
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

remove vault manager notifications from vault's notifications
4 participants
@turadg @samsiegart @dtribble @Chris-Hibbert