feat(inter-protocol): write liquidation auction results to Vstorage

[anilhelvaci]

closes: #8993

Description

The current state of the inter-protocol package lacks sufficient detail regarding the vault's liquidation process. This PR enhances visibility into vault liquidation by recording three stages on Vstorage:

• Pre-auction
• Post-auction
• Auction result

Refer to this snapshot for the defined structure of the Vstorage tree and the data being written to newly created nodes.

Key changes to the source code were focused on the vaultManager, including:

• the liquidateVaults method now receives the timestamp as an argument, which will be provided by the vaultDirector when the liquidationWaker is triggered.
• the utilization of allValuesSettled in the liquidateVaults method to resolve promises while ensuring the liquidation process proceeds despite any failures.
• the creation of the liquidations storage node at the start method, being the parent node the manager<id>.
• some helper methods were built to support the creation of storage nodes and writing to them, such as makeLiquidationVisibilityWriters, writeLiqVisibility, and makeLiquidationRecorderKits.

Security Considerations

We've two main security considerations;

• We're now making calls to two other vats inside liquidateVaults. Namely chainStorage and auctioneer.
  • chainStorage: To create new child nodes in the case of there are vaults to liquidate
  • auctioneer: Get the current schedule at a specific point in time
• Storing a variable to ephemera instead of this.state because we can't add a new property to it after initialization

Making calls to other vats

When making calls to other vats, we wanted to make sure we're not breaking the liquidations process in the case of a promise rejection arising from either chainStorage or auctioneer. So we've implemented allValuesSettled which doesn't throw even if any of those calls rejected. Here's part of the code we're exercising this feature;

const {
            makeDeposit: { userSeatPromise, deposited },
            liquidationVisibilityWriters,
            auctionSchedule,
          } = await allValuesSettled({
            makeDeposit,
            liquidationVisibilityWriters:
              helper.makeLiquidationVisibilityWriters(timestamp),
            auctionSchedule: schedulesP,
          });

And we handle the errors coming from chainStorage silently like below;

if (!liquidationVisibilityWriters) {
            trace(
              'writeLiqVisibility',
              `Error: liquidationVisibilityWriters is ${liquidationVisibilityWriters}`,
            );
            return;
}

We're not too worried about E(auctionPF).getSchedules() since it's result is just forwarded to the storage;

endTime: auctionSchedule?.liveAuctionSchedule.endTime,

The biggest consideration here to make is;

• !!! What happens if one of calls, or both, to auctioneer and chainStorage hangs forever instead of rejecting? Is there a timeout mechanism built for this?

If there's a chance that one those promises never resolve at all, we might end up blocking the liquidateVaults. In that case we might consider using E.when. Curious what OpCo is going to say about this.

Storing liquidationsStorageNode in ephemeral state

We decided to put all data related to liquidations under a storage node called liquidationsStorageNode. The way we keep track of this node was used to be through state variable by defining it as new property under this.state. However we've noticed that we're not able define new properties under this.state after it's initialization as described in virtual-objects.md. So we've found that storing our reference to liquidationsStorageNode in ephemera was working fine and was able to survive an upgrade. So our main assumption here is;

• The vaultFactory vat must be able to recover it's ephemera in case it loses its heap state. We know that it's possible during upgrades thanks to the provide pattern. However, we're not aware of how it can recover if the heap dumped for another reason.

Scaling Considerations

The main resource that can be impacted by this PR is the on-chain storage.
For each vaultManager, when an auction is initiated with liquidatable vaults, a new node is created under liquidations, being its key the start time of the open auction.
This node will have as children the auctionResult stage and vaults. Under vaults, it be the remaining two stages, preAuction and postAuction, detailed per liquidated vault.

Example of the Vstorage tree after an auction with start time set to 3600, see snapshot:

published
  vaultFactory
    managers
      manager0
        liquidations
          3600
            auctionResult
            vaults
              postAuction
                vault0
                ...
              preAuction
                vault0
                ...

It is important to highlight that, we've ensured by design, that the data written to each node does not grow.
Although, continuous monitoring of on-chain storage impact is necessary to address any unforeseen issues.

Documentation Considerations

The data being published for each auction liquidation stage should be documented, so the users and developers can understand the meaning of it, and how it could be used in their bidding strategies or integrations with other components.

Testing Considerations

For this PR, we built a set of unit tests for the new features implemented, as well as bootstrap tests to ensure that the contract behaves as expected when facing a contract upgrade or restart.
Test files:

• Unit tests
  • test-liquidationVisibility
• Bootstrap tests
  • test-liquidation-visibility
  • test-liquidation-compatibility

For the liquidation compatibility bootstrap test we were forced to copy the original source code so we could initiate the test environment with that version and later upgrade it with the current one.

The existing CI tests have passed successfully.

Upgrade Considerations

The changes made to the inter-protocol in this PR require an upgrade to the vaultFactory contract. With this in mind, we have built and tested a core-eval proposal, as it is mentioned above.

The vaultFactory proposal installation and execution were tested in the a3p-integration, confirming the expected behavior at the EVAL stage.

[Chris-Hibbert]

Thanks for the PR. Sorry it has taken us so long to get to this review.

[anilhelvaci]

Important for commit Jorge-Lopes@984f55b

As plan and vaultsInPlan are assigned implicitly tsc complained with TS2322 error. The assignment of plan and vaultsInPlan is tied to helper.planProceedsDistribution working without any errors and in the case of any error, the error is handled in the catch statement.

With this commit, we've introduced a new message in postAuctionRecorderKit and auctionResultsRecorderKit to indicate if plan or vaultsInPlan are not assigned properly for some reason, we print { error: 'Error distributing proceeds' }. Since there's no way I could find to produce this case in the tests, I had to proceed with no explicit tests. As all the other tests are passing, I believe it might be sufficient to leave it like this.

FYI @Chris-Hibbert

[anilhelvaci]

What branch should we target going forward? Ideally(for us) what ever code is running on the mainnet right now but I'm aware there might be some nuances over there. What do you think? @Chris-Hibbert

[Chris-Hibbert]

What branch should we target going forward? Ideally(for us) what ever code is running on the mainnet right now but I'm aware there might be some nuances over there. What do you think? @Chris-Hibbert

Please target master. The a3p-integration tests are supposed to reflect the current state of the chain. I believe it currently reflects the changes from our recent upgrade-15.

[anilhelvaci]

Problem when type checking causes CI to fail

The CI fails because of a failure in type checking. Here's the error message;

Error: test/vaultFactory/storage.test.js(147,64): error TS2345: Argument of type 'bigint' is not assignable to parameter of type 'number'.

And specific line is:

agoric-sdk/packages/inter-protocol/test/vaultFactory/storage.test.js

Line 147 in 944adea

t.is(quoteValue.amountOut.value / quoteValue.amountIn.value, highPrice);

The weird thing is the actual test runs with no problems. It's just that typescript is not able detect quoteValue.amountOut.value / quoteValue.amountIn.value is a bigint.

The other odd thing is, this PR has nothing to with the file throwing the error. And as far as I can see/figure the type definitions we add don't touch the definitions storage.test.js is using. This behavior does not happen in master. Could use some help.

cc @Chris-Hibbert @otoole-brendan

[Chris-Hibbert]

The CI fails because of a failure in type checking. Here's the error message;

It doesn't show up in test results for this PR. Do you have a link to the test log?

[anilhelvaci]

Oh sure, sorry about that => https://github.com/Jorge-Lopes/agoric-sdk/actions/runs/9613583517/job/26516739469

Just edited to replace the link with another one that has better error diagnostics.

[anilhelvaci]

Could you please approve CI workflows to run? @Chris-Hibbert

[Chris-Hibbert]

The weird thing is the actual test runs with no problems. It's just that typescript is not able detect quoteValue.amountOut.value / quoteValue.amountIn.value is a bigint.

It's weirder than that. Those lines of code haven't changed in your PR. On master, the type of the LHS of that comparison is any, same as in yours. I don't know why t.is() is fine with that in master, but wants a change for your PR.
@turadg, any idea why the lint results wouldn't be stable here?

[anilhelvaci]

If we explicitly declare the types as bigint, yarn lint:types works fine:

/** @type bigint */
const numerator = quoteValue.amountOut.value;

/** @type bigint */
const denominator = quoteValue.amountIn.value;

t.is(numerator / denominator, highPrice);

cc @Chris-Hibbert @turadg

[turadg]

I just came across this. I wasn't requested as reviewer. I don't think I have enough context to fully review but I notice some things that will need changing.

Also please squash the fixup commits into what they're fixing. E.g. fix(liquidationVisibility): rebase fixes does not belong in the commit history for master. This will require a force push but I notice you already are doing that.

[turadg]

the builds should not be checked in as they may drift from the sources. instead add the core-eval build output dirs to .gitignore

[anilhelvaci]

So are you suggesting;

• We build the bundles when running the test?
• Fetch them from a GH release ?
• Build them manually before running the tests and put them in some directory that its contents do not get pushed?

[turadg]

We build the bundles when running the test?

Sort of. We have a step that runs in CI to build whatever bundles need building: https://github.com/Agoric/agoric-sdk/blob/master/a3p-integration/scripts/build-all-submissions.sh

Here's an example that builds several core-evals:

agoric-sdk/a3p-integration/proposals/b:enable-orchestration/package.json

Lines 4 to 10 in 0fbeb93

	"source": "subdir",
	"sdk-generate": [
	"smart-wallet/build-wallet-factory2-upgrade.js",
	"testing/start-valueVow.js start-valueVow",
	"testing/restart-valueVow.js restart-valueVow",
	"vats/init-orchestration.js"
	]

The left side is the path under packages/builders/scripts and the right side is an optional local dir name.

[anilhelvaci]

Understood, started converting to submission with this commit bd8a071

[turadg]

there should never be a vaultFactoryV2.js in the SDK paths. There is simply vaultFactory.js which is whatever the HEAD version is. The earlier version is on-chain.

[anilhelvaci]

I understand where you are coming from. The reason I felt comfortable doing what you've highlighted is that vaultFactoryV2.js never persists to the next build as we run post.liquidation.js in the TEST phase of the a3p process. See here.

The reason why we make changes to the HEAD is, we are trying to trigger a liquidation. In order to test the things that depend on a liquidation we need to control the liquidation process. This means we need to have a timerService we can control which is used by both auctioneer and vaultFactory.

Triggering and controlling a liquidation is crucial for us as we try to test the state after a liquidation. I hope this justifies having a vaultFactoryV2.js in the SDK path in non-persisting TEST phase.

[turadg]

If it never persists, why put it under agoric-sdk/packages/inter-protocol? Can't you build it from somewhere in this proposal path?

[anilhelvaci]

Building vaultFactory outside of the sdk path seemed a little tricky at the time given all the dependencies and all that. Maybe I can copy interProtocol in the proposal path then build it there 🤔

[turadg]

Ah, true. So you need a special version of vaultFactory that's just for testing purposes? We havezcf.setTestJig for in-place testing logic, but a3p tests are for designed for production fidelity and so can't reach into that.

If this contract is just for testing, and not a hypothetical upgrade, then I don't see a problem with it being under agoric-sdk but with a name like 'vaultFactoryForTesting.js`. In which case I even think we should consider having it in master so it can be used again in the future and it doesn't stagnate from regular VF.

[anilhelvaci]

[DEPRECATED]

Please do not take into account what is being said in comment. #8994 (comment) has up to date content. @turadg

If this contract is just for testing, and not a hypothetical upgrade, then I don't see a problem with it being under agoric-sdk but with a name like 'vaultFactoryForTesting.js`. In which case I even think we should consider having it in master so it can be used again in the future and it doesn't stagnate from regular VF.

Yeah I can totally agree with that and believe this is to be the case for us. However, this stagnation problem keeps me from moving forward with creating a liquidation in a3p as auctioneer is also a part of this liquidation. The best solution I can think of is;

• Put manualTimer inside a contract and deploy it
  • creatorFacet will have methods for advancing time
  • publicFacet will hand out the reference to the manualTimer
• Spin up a new instance of auctioneer using the original auctioneer's installation(assuming auctioneer is not upgradable)
  • Have the fakeAuctioneer use manualTimer as its timer service
• Upgrade vaultFactory by using vaultFactoryForTesting.js as its entry point when bundling which uses manualTimer and fakeAuctioneer
  • The thing is we need vaultFactoryForTesting.js to have vaultFactory accept its timer service from privateArgs. I see that vaultFactory recently moved auctioneerPF to its private args. If we do the same for timer service, we won't need vaultFactoryForTesting.js at all.

I am curious about any suggestions you might have as well as what you think about this strategy.

[Chris-Hibbert]

Apologies for not making time to review this in detail. I've been assigned other tasks at higher priority. I look forward to spending time looking at this in details. To respond to some of your questions above:

!!! What happens if one of calls, or both, to auctioneer and chainStorage hangs forever instead of
!!! rejecting? Is there a timeout mechanism built for this?

If the auction doesn't respond, there's no way to automatically recovery.

The most plausible cause is that the auciton is busted or vaults has
the wrong OCap. In either of those situations, the best response is
restarting or re-configuring whatever is broken. In those situations,
it's understandable that liquidation won't take place, and it would be
a waste of effor for the vaultManagers to prepare vaults for liquidation.

The vaultFactory vat must be able to recover it's ephemera in case it loses its heap state. We
know that it's possible during upgrades thanks to the provide pattern. However, we're not aware of
how it can recover if the heap dumped for another reason.

The heap state can be lost when doing a full-upgrade, a null-upgrade, or just when a vat is paged out. "Null upgrade" just means intentionally starting from scratch without a change to the source code. The same restart takes place if a vat is paged out because of inactivity--this is less likely for auctions or vaults, but the mechanism is the same. In those cases, the code starts up by reading state from bagage, wihich includes the state of all durable kinds as well as other values recorded using provide().

The snapshot links are 404. Would you replace them with github permalinks? Those may become outdated, but that's better than broken links.

[anilhelvaci]

Apologies for not making time to review this in detail. I've been assigned other tasks at higher priority. I look forward to spending time looking at this in details. To respond to some of your questions above:

!!! What happens if one of calls, or both, to auctioneer and chainStorage hangs forever instead of
!!! rejecting? Is there a timeout mechanism built for this?

If the auction doesn't respond, there's no way to automatically recovery.

The most plausible cause is that the auciton is busted or vaults has the wrong OCap. In either of those situations, the best response is restarting or re-configuring whatever is broken. In those situations, it's understandable that liquidation won't take place, and it would be a waste of effor for the vaultManagers to prepare vaults for liquidation.

Yeah we have switched to Promise.all for this one after your first review.

The vaultFactory vat must be able to recover it's ephemera in case it loses its heap state. We
know that it's possible during upgrades thanks to the provide pattern. However, we're not aware of
how it can recover if the heap dumped for another reason.

The heap state can be lost when doing a full-upgrade, a null-upgrade, or just when a vat is paged out. "Null upgrade" just means intentionally starting from scratch without a change to the source code. The same restart takes place if a vat is paged out because of inactivity--this is less likely for auctions or vaults, but the mechanism is the same. In those cases, the code starts up by reading state from bagage, wihich includes the state of all durable kinds as well as other values recorded using provide().

Yeah, we right now rely on the start method of vaultManager to recover the ephemera.

The snapshot links are 404. Would you replace them with github permalinks? Those may become outdated, but that's better than broken links.

Done.

[anilhelvaci]

Overlap between vaultFactory upgrades

Currently a:upgrade-next upgrades vaultFactory and auctioneer in its EXECUTE phase, see:

agoric-sdk/golang/cosmos/app/upgrade.go

Lines 152 to 154 in a14b527

	vm.CoreProposalStepForModules("@agoric/builders/scripts/vats/add-auction.js"),
	// upgrade vaultFactory.
	vm.CoreProposalStepForModules("@agoric/builders/scripts/vats/upgradeVaults.js"),

When running a3p-integration tests, proposals run on the ghcr.io/agoric/agoric-sdk:unreleased image which includes current(whatever branch a3p tests are running on) version of agoric-sdk. In the context of this PR, current version is aimed to be Agoric:master and my tests are running on Jorge-Lopes:develop.

This situation leads to when a:upgrade-next upgrades vaultFactory, the features in this PR are also deployed. So d:liquidation-visiblity proposal ends up being merely a null upgrade. Here are some I can think of;

• If OpCo considers including liquidataion-visibility into a:upgrade-next then I suggest not having a separate d:liquidation-visibility proposal at all and implementing a liquidationVisibility.test.js under a:upgrade-next to test if our features work as expected. I expect, in this context, testing would require a fairly less overhead as upgradeVaults.test.js already triggers a liquidation and all we need to wait for that liquidation finish and we read its results. The thing to consider here, if we wait for the liquidation to end the test might take more 10 mins to complete.
• If OpCo decides to ship this PR in a later time with its own proposal then here's the work we need to do;
  • Write a new core eval code for vault factory upgrade since the one used for a:upgrade-next does some tricks with the auctioneer and price feeds, see upgrade-vaults.js
  • Trigger liquidation
    • For this we might follow the approach in a:upgrade-next due to the stagnation problems of our original approach (This comment explains from a high level). If we end up following a:upgrade-next's approach the TEST phase of the build will probably take around 5-15 mins.

Would appreciate a discussion about above.

cc @otoole-brendan @Chris-Hibbert @turadg

[otoole-brendan]

@anilhelvaci I think @turadg and/or @Chris-Hibbert are better equipped to answer this when they next get the opportunity