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.

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

Add a database for swap states #1243

Open
tcharding opened this issue Aug 20, 2019 · 2 comments

Comments

@tcharding
Copy link
Contributor

commented Aug 20, 2019

DONE: Added Sqlite infrastructure and an Sqlite backed MetadataStore.
TODO: Add an Sqlite backed StateStore.

This task now has the following associated issues, as the numbering implies these should most likely be done in order:

1. #1421
2. #1423
3. #1424

Currently cnd only stores swap states in memory so each new instance of cnd has no access to swaps done by a previous instance of cnd.

Spike 9 (https://github.com/comit-network/spikes/blob/master/0009-comit-btsieve-db.adoc) describes adding a database to cnd in order to save these states to disk. There seems to be no disagreement from the team on the recommendations within this spike. This issue is to implement the recommendations.

Note that it is within the scope of this ticket to define the schema for the database.
Additionally, you will need to create the initial migration script that initializes the database with the initial structure.
Diesel has support for database migrations and can even embed them into the binary (check the Recommendation section of the spike)
In other words: with the implementation of this ticket, making database changes should only require you to add another migration script, the "infrastructure" code for applying migrations should exist.

While working on this issue it was discovered that we should do a few things first in order to facilitate the work.

  • Move MetadataStore stuff out of swap_protocols
  • Separate/duplicate enums that are used for two different reasons within Metadata i.e., LedgerKind, AssetKind etc.

Tasks:

  • Implement an sqlite backed version of MetadataStore and StateStore
  • Uses cross platform location for database file i.e, uses directories crate.
  • Adds a command line option to pass in database file to use
  • Adds a command line option to initialize database i.e., delete the old one.
  • cnd reads previous swaps from database on startup.
@tcharding tcharding referenced this issue Aug 20, 2019
0 of 1 task complete

@thomaseizinger thomaseizinger changed the title Add a database for storing completed swaps Add a database for swap states Aug 21, 2019

@tcharding tcharding self-assigned this Aug 21, 2019

@tcharding

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

Adding a plan of attack here for comments or suggestions. Intend to first put together a draft PR with scope limited to swap metadata. This will mean clients can list all previous swaps but not get any information on them. If I understand correctly, this means only implementing an Sqlite backed version of MetadataStore. For API queries on a swap id that was created by a previous instance of cnd the query will return problem::state_store.

@thomaseizinger

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

Adding a plan of attack here for comments or suggestions. Intend to first put together a draft PR with scope limited to swap metadata. This will mean clients can list all previous swaps but not get any information on them. If I understand correctly, this means only implementing an Sqlite backed version of MetadataStore. For API queries on a swap id that was created by a previous instance of cnd the query will return problem::state_store.

Yes that seems reasonable.
I had another thought though and just realized that we may have been to quick in dismissing two of the tasks you added to the list above, sorry for that :(

At the moment, the e2e tests restart cnd and btsieve for each test to ensure an empty state. As soon as we have a database, that is no longer true. I think the e2e tests will actually break because the often assume that the list of swaps is empty in the beginning. Hence, you will need to modify the test harness to delete the database between the tests.
That in turn raises the question about the location of the database if several instances are running (do we want to have a data_dir commandline argument to override that?).

I think it would make sense to start with a PR that adds the following functionality:

  • create a database on startup if it does not exist yet
  • allows users to somehow override where it is stored (directories-rs has a data_dir so I'd suggest we add a commandline option to override that which we can use in the e2e tests)
  • create an initial "hello-world" migration that is executed as the cnd starts up

All of that can be done in a PR that can be merged right away. That way we have the necessary infrastructure in place that we need for the other usecases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.