Database and storage refactoring

[fboucquez]

Hi team,

I've been revisiting the wallet for the last week and I have seen a few points that I think they can be improved on.

If you have the time, please revisit it, I think there are valid points to be considered. I recommend checking out the code and running it locally, not just revisit the changed code in GitHub.

Data/Storage:

1. Models without typed attributes:  Components and the compiler cannot tell the attributes and the types of these values. Clients get the values by key and then they need to downcast to use them or ignore the types. Time is lost looking for the right attribute names.  Typos (invalid attribute name) and types (invalid operation on a value) errors are found at runtime.

   • You cannot tell the known attributes of Models like MosiacModel by just looking at it. Clients need to get a value using strings (e.g. Mosaic).
   • The name of the attributes are in table objects like MosaicsTable. Devs need to refer to this table which is time-consuming and error-prone.
   • This problem can be reduced by converting the model into another object like in the NamespaceModel.object. But this is another mapper returning another object with an anonymous type. Anonymous type is OK for local operations but when they are propagated in other parts of the code it's better to have a type name.

2. The Storage framework can be simplified: For a model object like Wallet you need  WalletModel, WalletTable, WalletRepository, etc. I understand the idea of having a more robust database but I don't think it's necessary in the Wallet Case. The data stored is either cache data (Network, Namespace, Mosaic) or almost-single-tenant configuration (Settings, Wallet, Account).

3. There is duplicated and boilerplate code: WalletsModel vs Namespaces - WalletRepository vs NamespaceRepository

Data/Storage improvements in the PR:

1. Migrate the storage object like Model and Table to just a simple POJO (not methods, just simple attributes) that can be serialized/deserialized to JSON. Examples are NamespaceModel, MosaicModel. The JSON schema as been kept for Account and Mosaic tables. No migration is required, they should when starting the new version. The Settings table has been slightly modified to fix the snake_case toward camelCase (example default_fee to defaultFee). The Table and Repository objects are not needed anymore and removed. Old storage classes have been also removed (they are kept in git history for reference).

2. The storage has been simplified to a generic object that can store/retrieve/delete an object of a given type SimpleObjectStorage

3. There is no storage related code duplicated for each table/model.

Data/Storage PR TODO:

The migration feature of the new storage hasn't been created. My plan is to add {version:x , data: RealStoredData} payload in the tables. This requires changing the table schemas a little bit. The version would be kept generic at the top level and not per row/entity inside the table.  Example: there is no need to keep a version per Account, all the stored accounts in the same table will have the same top-level version. In the case of migration, you migrate the whole lot. It shouldn't be possible to have an account of one version with an account of another version at the same time, everything should have the same version, or migrated from the same version

Modules separation of concern

Another issue I notice it's the high coupling between the different modules of the application. UI components can talk to service, store, repositories, etc. Stores may talk to repositories, service, etc. Services are aware of the vuex state.  Examples here, here, here, [here] (https://github.com/nemfoundation/symbol-desktop-wallet/blob/master/src/services/MosaicService.ts#L408)

If a function uses getter('someState') style, it may not be refreshed then when 'someState' is changed.

My recommendation is to define the dependencies a bit better: One example I'm trying to follow is (in general):

• UI Components: only get the status from State and getters: They can only announce operations via actions to the Stores.  
• Services are not aware of Vue. They can only talk to rest and the db storage for caching or configuration. if they don't know the $store, they will be simpler to unit test. Getting values using $store is a bad shortcut. as they are not well-typed and it's hard to tell from method contract level the values the service needs to perform an operation.
• Store is the glue between the Components and Services. They receive actions from UI components, perform an operation via services and change their state. Then the UI gets updated when the State is changed.

The best improvement example I've done is around Mosaic and Store where Components only gest the information via mapGetters

Added features in the PR:

• Known Peers nodes are added to the peer selector automatically after querying rest getNodePeers Hardcoded config peers are not loaded/refreshed from rest endpoint #139
• The Peer selector has a built-in  quick filter input (the list is pretty large now)
• The NetworkService and Network storage pings the known list to select the best available peer before creating the network repository. I have seen cases where the default peer is  banned
• There is a database cache for Mosaics and Namespaces. When the entities need to be loaded, the observable first sends to the clients the cached values, then it retrieves and sends the rest values. 1) data is shown if the peer is down, 2) faster user feedback/interaction
• NetworkConfiguration is loaded, preprocessed and cached. Then the components use the stored configuration to perform the operations Harcoded network config properties can be loaded from rest #140
• namespaceGracePeriodDuration is now a duration in seconds, not a block height Incorrect use of namespaceGracePeriodDuration #173
• SDK has been upgraded to 0.17.4-alpha. I removed an Axios call with an SDK call. No custom rest call is needed.  This means the wallet uses the official open API spec, the official catapult-rest can be used and not the fork
• Simplified the network.conf.json file and removed hardcoded testnet only code networkConfig.networks['testnet-publicTest'] calls like AboutPage. If the hardcoded configuration is kept to the minimal, the wallet can be easily ported to different network types or maybe support multiple network types at the same time. Most configuration should be retrieved from rest using just the default for the very initial boot.

Code improvements in the PR:

• Improved types veux State object. Now States like Mosaic, Namespace have better typed states, getters, mutations, actions, etc. When coding a UI component, and you need a store value, you can easily know the type of the value.
• Simplified action parameters allowing just one type. This avoids having multi-typed params and methods like here and here
• Improved anonymous types that are used across multiple components. The main example is the new Signer type. It was hard to tell the type of 'wallet/currentSigner'.
• With new Storage Models, the Model can be better reuse in UI Components simplifying the code as the attributes and the types are well known and reducing one-of types and mappers
• Removed $store from most services
• Only services talk to the database storage.
• Removed unnecessary downcast/upcast method in TransctionService method
• Incorrect use of WalletType enums. Wallet is using WalletType.fromDescriptor('Seed') when it can be simplified with just a WalletType.SEED

General TODOs (future tasks)

• Keep improving the separation between the different modules.
• CustomValidationRules and ValidationRuleset are still using the statically configured network properties/configuration.
• Types in State can still be improved by adding more types to actions. Is it possible to better type the component's mapGetters annotations having a compilation time error if the getter doesn't exist or it has a wrong type?
• When a Components perform an action like Logout, the UI component is responsible for telling the different stores the actions to be performed increasing coupling. When a new action needs to be performed when logging out, the component needs to be changed adding the dispatch call. I would like to use a more event base model, where one LOGOUT event is fire and different stores subscribe to that Event. One event handler could be the ''diagnostic' store that logs the events into Wallet console reducing the component responsibility to log the action.
• The same listener instance to the same peer can be used in the different Store increasing performance.
• symbol-hd-wallets needs to be upgraded 0.17.4-alpha so the SDK Account Types are compatible at compilation time without funny castings.
• Increase unit testing a lot. Not an easy task, although I reckon the PR code would be a bit simpler to unit tests (less coupling).  I would recommend enabling coveralls reports in Travis and GitHub
• Improve code documentation starting from the architectural classes (main, store, services, storage, etc) down to the UI components.
• Table and Transaction views are still using key-value dictionary and maps when better-typed objects could be used.

Summary:

I know this is a big PR and it has crossed multiple areas. It's not ideal and I do prefer having smaller PR but this is a special, my first take on the Wallet. We shouldn't be doing this.
Having said that, there are multiple ways of handling this PR.

1. One big PR merge after validating each point, regression testing, and feedback fixing. Then continue the TODOs as individual Issues and PRs
2. Split the PR into several ones a tackle one by one. It may take a lot of time to split. Each PR split may require regression testing that could take a lot of time. How to split is also hard, would we migrate half of the storage keeping both solutions at the same time?
3. Never merge this PR and takes some of the ideas into the code for future improvements and PR.

Happy to discuss each point.
Cheers

Fernando

[decentraliser]

Types in State can still be improved by adding more types to actions. Is it possible to better type the component's mapGetters annotations having a compilation time error if the getter doesn't exist or it has a wrong type?

There are solutions but they are a bit hacky (but better than nothing IMO). I think that there won't be further fixes before Vue 3 that will be written in TypeScript
see usage of GetterTree here

[decentraliser]

Table and Transaction views are still using key-value dictionary and maps when better-typed objects could be used.

About maps, another advantage of not using them is that Vue reactivity engine does not support them. However, Vue 3 will support them
see Observation Mechanism

[decentraliser]

Account import seems to be broken

When importing an account from mnemonic and chosing addresses (having the new default address with a balance and a transaction history), the dashboard left section (balance panel and mosaic list) initialize empty, and it throws:
vue.esm.js:1897 TypeError: Cannot read property 'plain' of null
at e.s (AccountBalancesPanel.vue?4d4a:7)
at e._render (vue.esm.js:3557)
at e.i (vue.esm.js:4075)
at ii.get (vue.esm.js:4488)
at new ii (vue.esm.js:4477)
at Rr (vue.esm.js:4082)
at e.xi.$mount (vue.esm.js:9063)
at e.xi.$mount (vue.esm.js:11974)
at init (vue.esm.js:3127)
at h (vue.esm.js:5989)

in walletChoose, the balances are not showing
After a moment (around 1s), the mosaics and the balance show, but the transaction list is empty

Wallets are not showing in the wallet view

Them when I logout and login, it goes straight to the mnemonic random generation screen (mousemove)

[decentraliser]

When starting the app with empty local storage

The language selector is not properly initialized (it shows as "Select" when starting the app with an empty localStorage)
When setting the language in that state, it throws:
vue.esm.js:1897 TypeError: Cannot read property 'accountName' of null
at v.SET_SETTINGS (AppInfo.ts:128)
at Array. (vuex.esm.js:747)
at v.dispatch (vuex.esm.js:438)
at v.dispatch (vuex.esm.js:332)
at T.n.dispatch (vuex.esm.js:675)
at v.SET_LANGUAGE (AppInfo.ts:137)
at Array. (vuex.esm.js:747)
at v.dispatch (vuex.esm.js:438)
at v.dispatch (vuex.esm.js:332)
at e.set [as language] (LanguageSelectorTs.ts:62)

[decentraliser]

Listeners do not seem to work

There are no notifications for new (un)confirmed transactions
password1
When entering in "Create sub namespace" tab, the page stays black, with the error:
vue.esm.js:1897 TypeError: Cannot read property 'length' of undefined
at fn (CreateSubNamespace.vue?fc2f:1)
at i (vue.esm.js:2599)
at e.Ut [as _t] (vue.esm.js:2695)
at e.o (AssetFormPageWrap.vue?1dd1:1)
at e._render (vue.esm.js:3557)
at e.i (vue.esm.js:4075)
at ii.get (vue.esm.js:4488)
at new ii (vue.esm.js:4477)
at Rr (vue.esm.js:4082)
at e.xi.$mount (vue.esm.js:9063)

[decentraliser]

Assets tables

In mosaics table, when clicking "link", the mosaic name does not appear
In namespace table, mosaics do not show in the dropdown when clicking "link"
In mosaics table, when clicking "link", the namespaces that shows are not the ones of the active wallet

[decentraliser]

If I have a bad connectivity, a node switch can let me forever with the loading overlay

[decentraliser]

For some reason, the npm run dev command is making my browser crash at the first load of localhost

I tried this branch with the static files generated by npm run build, after an npm install, and I tried to reproduce the bugs on master the same way

[decentraliser]

transaction list
Sometimes, when switching wallets, the transaction list is not properly initialize
Wallet change can be done from the wallet section, or from the dashboard using the wallet selector

The transaction list sometimes does not clear when switching wallets (when switching to wallet B, I still see transactions of wallet A) untill switching to another view and coming back to the dashboard

[fboucquez]

transaction list
Sometimes, when switching wallets, the transaction list is not properly initialize
Wallet change can be done from the wallet section, or from the dashboard using the wallet selector

The transaction list sometimes does not clear when switching wallets (when switching to wallet B, I still see transactions of wallet A) until switching to another view and coming back to the dashboard

I haven't revisited the transaction lists. Something that I have noticed is that transactions aren't been cached in db like mosiac or namespaces. Is this expected?

What I have done for namespaces and mosaic is loading the entities for all known wallets (sdk allows you to load entities for mutliple accounts with one rest call). Then when switching from one wallet to another, the new wallet has the namespaces/mosaics prefetched, feedback to the user is quicker. There will be another loading just to retrieve the latest ones when hitting the page (or maybe in response to another event) I guess we could do something similar with transactions instead of just cleaning up existing and load new ones.

[fboucquez]

This PR will need a full round trip of regression testing and fixes especially after all the merges pre-release. I just wanted to be sure the team is ok with the path taken in order to allocate more time.

[fboucquez]

Listeners do not seem to work

There are no notifications for new (un)confirmed transactions

Listeners seem working, I'm announcing transactions

[decentraliser]

do the snackbars show when tx get unconfirmed and confirmed? and are the new transactions added to the transaction list? It wasn't working when I tried Le mer. 15 avr. 2020 à 10:51, fboucquez <[email protected]> a écrit :

…

Listeners do not seem to work There are no notifications for new (un)confirmed transactions Listeners seem working, I'm announcing transactions — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#177 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIWPBPZNRH23EJ7GBBBISXTRMUOKPANCNFSM4LXFPYOA> .

[fboucquez]

Hi @decentraliser, please have another look at this PR when you have the chance

1. Fixed language selector
2. Fixed Mosaic and Namespace tables link
3. Simplified MosaicAmountDisplay. Clients don't need to do the absolute/relative calculation or send the right divisibility. It's handled in the component. (Something similar with the ticker)
4. Created Transaction module to handle confirmed/unconfirmed/partial transactions and blocks. I think the loaders has been simplified. Transaction dashboard loading is fine now. How blocks are loaded in my PR https://github.com/NEMStudios/symbol-desktop-wallet/blob/storage-refactoring/src/store/Transaction.ts#L179
5. I've revisited the listeners and they seem to be working. Maybe I've missed something?
6. I haven't got time to test account reimport.
7. I couldn't reproduce the loading crash. What browser are you using?
8. Custom throttling like RESTDispatcher and TransactionListOptionsTs.refreshStream$: are not used anymore. We are discussing a better way of throttling at SDK level (or when rest is actually called). The rest being banned problems have been improved in the server, now we may have too many request errors

[decentraliser]

7. I couldn't reproduce the loading crash. What browser are you using?
   Chrome, it seems OK now, might have been caused by one of the bugs you just fixed

[decentraliser]

When creating an account, still can't see the wallet in the list, and the wallet selector does not show

And when re-logging, it goes to generateMnemonic again, should be a bug somewhere

[decentraliser]

Others seems good. Maybe make one cycle on wallet creation / import, and then I will do an exhaustive review of all features

[decentraliser]

To reproduce the navigator crashing, you can try to select the node jp12.nemesis.land
this is an https node, and queried with http://
I think it would be easier to stick to the official nodes first to reduce the difficulty of this PR

[fboucquez]

Hi @decentraliser @evias

The fixes in the last commits:

1. Improved performance on peer change, exception handling and the fallback
2. Added address alias resolution to transaction list and detail
3. Improved types around transaction views
4. Mosaic Table show owned mosaic (including when the balance is zero) and other mosaics when the current user has balance.

Could you give it another go?

[vincent-lee90]

hey,teamate!
i will suggest that if you want to change the iview styles in this component.please add scoped,like this <style lang="less" scoped></style>

[evias]

ACK

[vincent-lee90]

I didn't understand the type of prop address is Address | NamespaceId, but the value of transaction && transaction.recipientAddress is Boolean

[evias]

ACK