Jorge-Lopes / agoric-sdk

monorepo for the Agoric Javascript smart contract platform
Apache License 2.0
0 stars 0 forks source link

Tasks before opening the original PR #26

Closed anilhelvaci closed 6 months ago

anilhelvaci commented 6 months ago
Jorge-Lopes commented 6 months ago

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

PR description:

closes: #XXXX refs: #XXXX

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:

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:

Security Considerations

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:

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.

Jorge-Lopes commented 6 months ago

Security Considerations

Does this change introduce new assumptions or dependencies that, if violated, could introduce security vulnerabilities? How does this PR change the boundaries between mutually-suspicious components? What new authorities are introduced by this change, perhaps by new API calls?

anilhelvaci commented 6 months ago

Security Considerations

We've two main security considerations;

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; https://github.com/Jorge-Lopes/agoric-sdk-liquidation-visibility/blob/672cf1240c619383ea71f383614b4aabdeb06280/packages/inter-protocol/src/vaultFactory/vaultManager.js#L1302-L1311

And we handle the errors coming from chainStorage silently like below; https://github.com/Jorge-Lopes/agoric-sdk-liquidation-visibility/blob/672cf1240c619383ea71f383614b4aabdeb06280/packages/inter-protocol/src/vaultFactory/vaultManager.js#L722-L728

We're not too worried about E(auctionPF).getSchedules() since it's result is just forwarded to the storage; https://github.com/Jorge-Lopes/agoric-sdk-liquidation-visibility/blob/672cf1240c619383ea71f383614b4aabdeb06280/packages/inter-protocol/src/vaultFactory/vaultManager.js#L697

The biggest consideration here to make is;

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;

Jorge-Lopes commented 6 months ago

@anilhelvaci we have built 2 unit test to cover those scenarios were on of the promises is rejected

Link to tests: https://github.com/Jorge-Lopes/agoric-sdk-liquidation-visibility/blob/672cf1240c619383ea71f383614b4aabdeb06280/packages/inter-protocol/test/liquidationVisibility/test-liquidationVisibility.js#L1202-L1486

anilhelvaci commented 6 months ago

All tasks completed, closing this one.