-
Notifications
You must be signed in to change notification settings - Fork 465
WIP token_orderbook_snapshots schema #1338
WIP token_orderbook_snapshots schema #1338
Conversation
@@ -158,6 +158,25 @@ const transactions = new Table({ | |||
], | |||
}); | |||
|
|||
const token_orderbook_snapshots = new Table({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Migrations are supposed to be immutable. In order to guarantee schema consistency, you can't change any existing migration files. Can you create a new migration for creating this table instead?
{ name: 'retrieval_timestamp', type: 'bigint' }, | ||
{ name: 'order_type', type: 'char(42)' }, | ||
|
||
{ name: 'base_asset_symbol', type: 'char(42)' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use either base_asset
and base_volume
or in_symbol
and volume_in
. Mixing these naming conventions makes it unclear which volume corresponds to which asset.
{ name: 'base_asset_address', type: 'char(42)' }, | ||
{ name: 'volume_from', type: 'varchar' }, | ||
|
||
{ name: 'quote_asset_symbol', type: 'char(42)' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think quote_asset_symbol
and base_asset_symbol
should have type varchar
. We don't know for sure that they will be constrained to any particular length.
const token_orderbook_snapshots = new Table({ | ||
name: 'raw.orderbook_snapshots', | ||
columns: [ | ||
{ name: 'source', type: 'char(42)' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, source
should be of type varchar
.
columns: [ | ||
{ name: 'source', type: 'char(42)' }, | ||
{ name: 'retrieval_timestamp', type: 'bigint' }, | ||
{ name: 'order_type', type: 'char(42)' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
order_type
should be exactly either buy
or sell
. Arbitrary strings are not allowed. Can we use an enum type here?
|
||
{ name: 'base_asset_symbol', type: 'char(42)' }, | ||
{ name: 'base_asset_address', type: 'char(42)' }, | ||
{ name: 'volume_from', type: 'varchar' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
volume_from
and volume_to
should have type numeric
, which is a number type with arbitrary precision. This allows us to do math in queries and avoids floating point precision issues and rounding errors as much as possible.
f02195f
to
e749050
Compare
Description
Testing instructions
Types of changes
Checklist:
[WIP]
if necessary.