vulpemventures / ocean

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

Db schema update, SetAccountLabel RPC impl, test refactor #45

Closed sekulicd closed 1 year ago

sekulicd commented 1 year ago

Changes:

  1. Main thing was to add primary key namespace column in account table and label column (account table as well)
  2. Tables account_script_info, tx_input_account,utxo are updated with ref(fk_account_namespace) to account table
  3. Few commands were added to the Makefile that are used to work easier with db, vet_db -> checks if up and down mig scripts are ok
  4. DB layer tests are moved to the new location /test and test suites are added as necessity to isolate state for each test case (run before/after code where needed) and to prepare/clean db for each test suite, also whit this some issues with race conditions disappeared
  5. RPC SetAccountLabel is added that updates account label
  6. Other stuff that are changed in other layers(app, interface, cli) are just renaming name to namespace and adding label
  7. Race issue is fixed with 188f55e, isLocked was not protected with mutex in sendReport method.

This closes #44 This closes #15

@tiero @altafan please review.

sekulicd commented 1 year ago

I see most of comments are related to the request to abstract away from user namespace/label columns into one called(account, accountName, name). For UX perspective this is better option but it adds bit of complexity to the backend, since we dont know wether user passed namespace or label we need to have two maps accountsPerNamespace and accountsPerLaber in order to support search "1 in 2(one field that could mean two things)", we need to update repositories as well. Field previously called accountName was all over the code base and wanted to be consistent, so the code is more readable and that person who reads it can recognise same arg name everywhere wether it is in scanners or repos or somewhere else.

So se this was already big change, it we want to go with the requirement to allow user to search in one filed using namespace/label please suggest how we want to call and i suggest to use it everywhere.

WDYT @tiero @altafan ?

tiero commented 1 year ago

Would starting with namespace only enough for time being? Will refactor later to add label?

altafan commented 1 year ago

So se this was already big change, it we want to go with the requirement to allow user to search in one filed using namespace/label please suggest how we want to call and i suggest to use it everywhere.

Let's refer internally to an account just as account, like for example in utxos, transactions, scanners etc. Let's use instead name to allow the user refer to an account (protobuf), whether he's passing a namespace or label. Let's keep using the flag --account-name without changing it. The only place where we need to make an explicit distinction between namespace and label is in the creation of the wallet and for setting an account label.

@sekulicd do you agree?

sekulicd commented 1 year ago

@altafan please check now.

altafan commented 1 year ago

This contains too many unnecessary changes. We can eventually recover the work done here to refactor the tests to open a dedicated PR. Closing in favour of #49.