vulpemventures / ocean

:ocean: Elements/Liquid wallet daemon
MIT License
5 stars 7 forks source link

Add account immutable Namespace and optional Label #49

Closed altafan closed 1 year ago

altafan commented 1 year ago

This adds an immutable and non-null Namespace for the wallet account, generated automatically by ocean with the format bip<root_path_purpose>-account<index>. This field is used internally to refer to wallet accounts, like for example in utxo or transaction domains.

This also adds a mutable and nullable field Label field that the user can set when creating a wallet account or later with the a brand new SetAccountLabel API.

The user can use either the namespace or the label (if set) to refer to a wallet account in all other APIs.

Closes #42. Closes #44.

NOTE: this includes an hotfix in electrum blockchain scanner to prevent race conditions. Credits to @sekulicd who discovered this in #45.

Please @tiero @sekulicd review this.

altafan commented 1 year ago

1.1 We didnt add new migration file, with new changes, but update existing one, this means we need to destroy/recreate already deployed DB's which is maybe not critical if we are not in testnet/prod

I opted for this solution since we don't have any service using ocean in prod yet. But anyway, I was wondering if adding new migration files was enough: what happens if an existing ocean wallet's db that has several records with old table format and Name field gets updated? Are those records updated accordingly? Are they somehow corrupted after the table update?

1.2. We didnt make relations to the other tables(tx_input_account, utxo) which lowers integrity of db data

This wasn't done even before these changes. If this is a bug, we should open an issue and fix it with a dedicated PR since it goes out of the scope of this very one.

1.3 label in account table is not unique and it is referenced in DELETE query which means one could delete many account if those have same name

Label is not unique into account table but this is ensured by the domain logic. There, we make sure that account labels are unique. But in general, I think the change to the DeleteAccount query can be reverted since we first get the account via label and always refer to it via namespace when deleting (check here). I'm gonna revert this change.

altafan commented 1 year ago
  1. Inconsistency: We wanted to give user option to reference account name with label or namespace throughout different APIs and that is ok, however having same var name meaning different stuff in code base is something i found problematic

I don't think we can talk about inconsistency if variables are named differently in components.

I didn't make such changes in scanners, repos, etc. to prevent having tons of files modified only for some variable naming, which is not really important. After all, if the code is well written, a change of a variable name should not be so confusing.

altafan commented 1 year ago
  1. Also testing was improved in rejected PR, and these changes were introduced with necessity as adding references to other tables required more job for preparing/cleaning tests for DB, and this preparation/cleaning can be different for different repos, also it had sanity test(kind of end to end)

Again, even if I appreciated the effort, it's not part of the scope of this PR and should be done in a dedicated PR by prior discussing on how to make the changes.

altafan commented 1 year ago

1.3 label in account table is not unique and it is referenced in DELETE query which means one could delete many account if those have same name

Reverted changes to DeleteAccount query.

sekulicd commented 1 year ago

I opted for this solution since we don't have any service using ocean in prod yet. But anyway, I was wondering if adding new migration files was enough: what happens if an existing ocean wallet's db that has several records with old table format and Name field and gets updated? Are those record updated accordingly? Are they somehow corrupted after the table update?

Purpose is to keep existing data and add new things(columns, constraints etc)

altafan commented 1 year ago

Purpose is to keep existing data and add new things(columns, constraints etc)

But the existing account records with deprecated Name field would not be supported anymore. I think this should be something handled by the migration somehow.