Address table

[WhoSoup]

Just saw the following:

pegnetd/node/pegnet/addresses.go

Lines 8 to 72 in dd4aec9

	const createTableAddresses = `CREATE TABLE "pn_addresses" (
	"id" INTEGER PRIMARY KEY,
	"address" BLOB NOT NULL UNIQUE,
	"peg_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("peg_balance" >= 0),
	"pusd_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("pusd_balance" >= 0),
	"peur_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("peur_balance" >= 0),
	"pjpy_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("pjpy_balance" >= 0),
	"pgbp_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("pgbp_balance" >= 0),
	"pcad_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("pcad_balance" >= 0),
	"pchf_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("pchf_balance" >= 0),
	"pinr_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("pinr_balance" >= 0),
	"psgd_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("psgd_balance" >= 0),
	"pcny_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("pcny_balance" >= 0),
	"phkd_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("phkd_balance" >= 0),
	"pkrw_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("pkrw_balance" >= 0),
	"pbrl_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("pbrl_balance" >= 0),
	"pphp_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("pphp_balance" >= 0),
	"pmxn_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("pmxn_balance" >= 0),
	"pxau_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("pxau_balance" >= 0),
	"pxag_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("pxag_balance" >= 0),
	"pxbt_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("pxbt_balance" >= 0),
	"peth_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("peth_balance" >= 0),
	"pltc_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("pltc_balance" >= 0),
	"prvn_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("prvn_balance" >= 0),
	"pxbc_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("pxbc_balance" >= 0),
	"pfct_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("pfct_balance" >= 0),
	"pbnb_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("pbnb_balance" >= 0),
	"pxlm_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("pxlm_balance" >= 0),
	"pada_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("pada_balance" >= 0),
	"pxmr_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("pxmr_balance" >= 0),
	"pdas_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("pdas_balance" >= 0),
	"pzec_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("pzec_balance" >= 0),
	"pdcr_balance" INTEGER NOT NULL
	CONSTRAINT "insufficient balance" CHECK ("pdcr_balance" >= 0)
	);
	`

That defeats the whole purpose of using relational databases in the first place. You have to add/remove columns to modify the assets (leading to data loss and backward incompatibility) and you can't run automated queries. This is what k-v dbs are for.

The SQL way of doing it would be three tables:

1. An address / id mapping (this is only for FATd compatibility, the address is already a unique id)
2. An asset / assetid mapping table (eg 1 is USD, 2 is PEG, 3 is EUR, ...)
3. A balance table that consists of (addressid, assetid, balance) with a unique key of (addressid, assetid)

that would let us remove assets without having to remove records from the database and also allow easier comparative queries, like querying a list of assets sorted by their supply

[Emyrk]

While I agree with you @WhoSoup, I believe this is the table format that Adam agreed with.

I will have to dig up the conversion and the rational.

[WhoSoup]

Using this format means we have to generate on-demand sql queries instead of being able to use prepared statements, since our list of currencies is mutable. We'd also have the table definition change over time so the table creation query itself has to be mutable. That would break with the standards (and particularly db verification) of FATd, so I'd really like @AdamSLevy to take a look at this.

[Emyrk]

@sambarnes Do you remember that convo with Admin?

[sambarnes]

@Emyrk Really wasn't that long of a conversation. Was in the FAT Integration DM:

me: Starting to work on a database schema for the balances. Thinking of just mimicking current db/addresses but having the ~30 separate balance columns in there. Objections before I get too far into implementing it?

adam: I am a fan of additional columns for the addresses tables

I don't really have a strong opinion. This was just quick and dirty to get up and running and let us start populating balances. I don't imagine we'll be adding/removing assets while pegnetd is a thing, but that might happen later (months down the line) when we're in FATd already. We should definitely use the route you suggest at that point in time

And like I said, I've been in nosql land for a few years now and haven't been thinking in SQL. So if you think its not the way to go @WhoSoup then I trust your judgement.

We already have that mapping you mention as iotas in pegnetd/fat/fat2/pticker.go.

[Emyrk]

I think for the current daemon, either option will work.

I am leaning towards the, "it's implemented, so let's just stick with it for now"

[AdamSLevy]

I believe what @WhoSoup suggested is the proper SQL fully normalized way of approaching this problem. It allows for more dynamically adding more currencies over time without a schema change. He is also right that hard coding the currencies to columns limits the ability to use fully prepared statements. It will become important to control when currencies become accepted by the network, so with this approach I also recommend adding an activation height column to the currencies/assets table. These are things I didn't think about when we had this brief discussion earlier.

Of course the other thing to keep in mind is that it would be wise to keep the normalized ids for currencies match the values chosen for the golang ticker type that describes the type of asset: https://github.com/Factom-Asset-Tokens/fatd/blob/9287bc808c49019296d455109e96f34ba521c6a2/fat/fat2/pticker.go#L6-L40

On the other hand, hard coding the currencies as columns is simpler (sort of) and it sounds like its done.

A couple notes on fatd. Currently I validate the database schema extremely rigorously when I open connections. In fact schemas that are actually identical but have differing whitespace will fail this validation. I don't believe it was a good design choice long term and I plan to change this to just checking schema_version and user_version and let any schema errors that misses come up when the query occurs.

Additionally, while trying to avoid schema changes is a good design goal, it's not ultimately feasible indefinitely. I expect to need to add a migration mechanism for fatd. Sub modules like FAT token types or PEG net may need to be able to apply their own custom migrations when databases are opened.