-
Notifications
You must be signed in to change notification settings - Fork 13
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
Postgres database #710
base: v1
Are you sure you want to change the base?
Postgres database #710
Conversation
PR Summary
|
@altafan please review proposed pg db schema. |
internal/infrastructure/storage/db/pg/migration/20230504102232_init.up.sql
Outdated
Show resolved
Hide resolved
internal/infrastructure/storage/db/pg/migration/20230504102232_init.up.sql
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,63 @@ | |||
CREATE TABLE market ( | |||
name VARCHAR(50) NOT NULL PRIMARY KEY, |
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.
This implies that in other layers (domain, interface) we MUST validate the name specified by the user and check that length is <= 50.
internal/infrastructure/storage/db/pg/migration/20230504102232_init.up.sql
Outdated
Show resolved
Hide resolved
internal/infrastructure/storage/db/pg/migration/20230504102232_init.up.sql
Outdated
Show resolved
Hide resolved
quote_asset VARCHAR(255) NOT NULL, | ||
base_asset_precision INTEGER NOT NULL DEFAULT 8, | ||
quote_asset_precision INTEGER NOT NULL DEFAULT 8, | ||
tradeable BOOLEAN NOT NULL DEFAULT FALSE, |
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.
This is not really a typo, but the field at domain level is called Tradable, so let's stick with it. Also, we can remove defaults.
tradeable BOOLEAN NOT NULL DEFAULT FALSE, | |
tradable BOOLEAN NOT NULL, |
internal/infrastructure/storage/db/pg/migration/20230504102232_init.up.sql
Outdated
Show resolved
Hide resolved
internal/infrastructure/storage/db/pg/migration/20230504102232_init.up.sql
Outdated
Show resolved
Hide resolved
internal/infrastructure/storage/db/pg/migration/20230504102232_init.up.sql
Outdated
Show resolved
Hide resolved
internal/infrastructure/storage/db/pg/migration/20230504102232_init.up.sql
Outdated
Show resolved
Hide resolved
internal/infrastructure/storage/db/pg/migration/20230504102232_init.up.sql
Outdated
Show resolved
Hide resolved
internal/infrastructure/storage/db/pg/migration/20230504102232_init.up.sql
Outdated
Show resolved
Hide resolved
internal/infrastructure/storage/db/pg/migration/20230504102232_init.up.sql
Outdated
Show resolved
Hide resolved
); | ||
|
||
CREATE TABLE swap ( | ||
id TEXT PRIMARY KEY, |
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.
maybe better?
id TEXT PRIMARY KEY, | |
id VARCHAR(255) PRIMARY KEY, |
name = $7 RETURNING *; | ||
|
||
-- name: DeleteMarket :exec | ||
DELETE FROM market WHERE name = $1; |
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.
Are the related fees deleted as well this way?
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.
not with that only, but yes in repo method, check in here
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.
since we opted for locigal deletion of markets, this is the best fit for out needs.
A suggestion for the future for the physical deletion scenario, better to let pg handle this by adding a DELETE ON CASCADE
to the primary key of the table used as fk in others.
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.
@sekulicd i noticed that the trade table doesn't contain any market-related field, but rather there's a relation between market and trade tables to retrieve them when querying for trades.
There are a bunch of problems with this approach:
- if a market is deleted, the trades made against it will forever miss market-related info. This can be fixed by deleting markets logically instead of physically by adding an
active
bool field that defaults to true. - market fees and price must be stored in the trade table because they represent fees and prices at the time the trade took place. Instead, by keeping a relation between the 2 tables, we might return incorrect values, since price and fees can be subject to changes (in particular the price!) during the life of a market.
base_asset_precision INTEGER, | ||
quote_asset_precision INTEGER, | ||
tradable BOOLEAN, | ||
strategy_type INTEGER, | ||
base_price DOUBLE PRECISION, | ||
quote_price DOUBLE PRECISION, |
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.
These fields don't need a default but must not be null.
base_asset_precision INTEGER, | |
quote_asset_precision INTEGER, | |
tradable BOOLEAN, | |
strategy_type INTEGER, | |
base_price DOUBLE PRECISION, | |
quote_price DOUBLE PRECISION, | |
base_asset_precision INTEGER NOT NULL, | |
quote_asset_precision INTEGER NOT NULL, | |
tradable BOOLEAN NOT NULL, | |
strategy_type INTEGER NOT NULL, | |
base_price DOUBLE PRECISION NOT NULL, | |
quote_price DOUBLE PRECISION NOT NULL, |
status_code INTEGER, | ||
status_failed BOOLEAN, |
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.
these fields don't need defaults but they must be not null.
status_code INTEGER, | |
status_failed BOOLEAN, | |
status_code INTEGER NOT NULL, | |
status_failed BOOLEAN NOT NULL, |
withdrawalRepository domain.WithdrawalRepository | ||
} | ||
|
||
func NewService(dbConfig DbConfig) (ports.RepoManager, error) { |
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.
to make things much simpler, we can expect here 2 strings, one being the pg connect URL in the formal postgresql://user:password@host:port/name
and the other being the migration source path.
This way we can expose just 2 env vars at config level TDEX_PG_CONNECT_ADDR
and TDEX_PG_MIGRATION_SOURCE
. This of course implies validating the pg connect addr, which I think it's fine to be done here in this method.
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.
@altafan are u sure about this? in all project we used env var for each db info, changing this also changes out deployment habits a bit
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.
Yes, definitely better to use 1 env var rather than 4. In the end, the SDK uses the string URL too instead of 4 distinct params. I'd like to make the same changes in other projects too to simplify the configuration.
This adds pg db implementation.
@altafan please review.