fix: change server #411

r4mmer · 2023-08-31T16:04:56Z

Acceptance Criteria

Use the correct methods to change the server and ws server
When changing networks, clean the registered tokens as well.

Security Checklist

Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

codecov · 2023-08-31T16:07:16Z

Codecov Report

Patch coverage: 2.50% and project coverage change: -0.05% ⚠️

Comparison is base (9a477a3) 9.41% compared to head (5480cd8) 9.36%.

Additional details and impacted files

@@           Coverage Diff            @@
##             dev    #411      +/-   ##
========================================
- Coverage   9.41%   9.36%   -0.05%     
========================================
  Files        114     114              
  Lines       5097    5123      +26     
  Branches     677     684       +7     
========================================
  Hits         480     480              
- Misses      3975    3994      +19     
- Partials     642     649       +7

Files Changed	Coverage Δ
src/sagas/wallet.js	`0.74% <0.00%> (-0.02%)`	⬇️
src/screens/LoadWallet.js	`0.00% <ø> (ø)`
src/screens/Server.js	`0.00% <0.00%> (ø)`
src/screens/TransactionDetail.js	`0.00% <0.00%> (ø)`
src/storage.js	`12.92% <0.00%> (-0.46%)`	⬇️
src/utils/wallet.js	`3.33% <0.00%> (-0.05%)`	⬇️
src/reducers/index.js	`22.50% <50.00%> (-0.10%)`	⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

luislhl · 2023-09-04T22:00:34Z

src/utils/wallet.js

+
+    if (networkChanged) {
+      // need to clean the storage, including registered tokens.
+      await wallet.storage.cleanStorage(true, true, true);


Why don't we make the wallet.stopmethod above accept a new cleanTokens param instead of calling the wallet.storage directly?

Stopping a wallet should not remove registered tokens, this is necessary here because we will be moving to another network where these tokens do not exist and leaving the tokens on the storage would make the wallet screen show tokens that do not exist on the connected network.

My question is not about the logic, I think the logic is great.

It's just that something like this could be better, because it would keep the pattern already in use for the other options:

async changeServer(wallet, pin, routerHistory, networkChanged) { // We only clean the storage if the network has changed const options = { cleanStorage: true, cleanAddresses: true }; if (networkChanged) { options["cleanTokens"] = true; } await wallet.stop(options);

This way we don't have to call the wallet.storage directly, and we keep the option available and documented in the stop method in case anyone needs it as well.

Created this issue to track this change on the wallet-lib HathorNetwork/hathor-wallet-lib#575

r4mmer · 2023-09-13T13:18:53Z

src/sagas/wallet.js

+    } else {
+      console.error(e);
+      // Return to locked screen when the wallet fails to start
+      LOCAL_STORE.lock();
+      routerHistory.push('/locked/');


Sometimes the wallet would be stuck on loading addresses, the error I got was that the configured network and server network did not match, it was raised during the start, but we already have a try..catch it just didn't handle the error, so the sagas would be waiting on a wallet that was not started.

The solution here is just to go back in the locked screen, but we can also add an error message in the future.
The reason I was having this error was because the change server error this PR solves would change the server but did not change the network so I do not expect to have this error again but i'm leaving this to make debugging future errors of this nature easier.

r4mmer · 2023-09-13T14:01:35Z

src/screens/TransactionDetail.js

+      for (const output of data.tx.outputs) {
+        if (!output.token) {
+          if (output.token_data === 0) {
+            output.token = hathorLib.constants.HATHOR_TOKEN_CONFIG.uid;
+          } else {
+            output.token = data.tx.tokens[(output.token_data & hathorLib.constants.TOKEN_INDEX_MASK) -1].uid;
+          }
+        }
+      }
+
+      for (const input of data.tx.inputs) {
+        if (!input.token) {
+          if (input.token_data === 0) {
+            input.token = hathorLib.constants.HATHOR_TOKEN_CONFIG.uid;
+          } else {
+            input.token = data.tx.tokens[(input.token_data & hathorLib.constants.TOKEN_INDEX_MASK) -1].uid;
+          }
+        }
+      }


getFullTxById fetches the transaction from the api, which uses has a slightly different structure from the websocket transactions and transactions received via history sync.
These lines just add the token uid where the helper will need it.

The issue here was that not having the token would make the transaction balance think that there was only a single token of uid undefined.

I'm not sure I get this: would we have some problem if we implemented this in the lib itself, so the getFullTxById method always returns with the right token?

I mean, when using this method in other wallets, won't they need to perform a similar treatment to this response?

You are absolutely right, the only other place this is used is on the decode api of the headless and when it is used the token would be missing. I created a PR to fix this on the headless HathorNetwork/hathor-wallet-headless#342

The main problem in my opinion is that the same transaction can have multiple structures depending on which api is used to fetch it from the fullnode or if it comes from the websocket.
Maybe the best solution would be to normalize the transaction model sent by the fullnode and the wallet-lib would be more stable.
Since solving this on the wallets is the simpler approach I think we should stick with it for now.
What do you think?

I think it's ok to stick to the simpler approach. But we should have an issue to implement the best solution in the future.

src/reducers/index.js

luislhl · 2023-09-13T20:28:26Z

src/screens/TransactionDetail.js

@@ -94,6 +94,25 @@ class TransactionDetail extends React.Component {

    try {
      const data = await this.props.wallet.getFullTxById(this.props.match.params.id);
+      for (const output of data.tx.outputs) {


Should we have unit tests for this?

As a suggestion, it might be a good idea to avoid adding too much business logic here so we have a clearer separation between presentation layer components and business logic (something like https://blog.bitsrc.io/improve-react-component-maintainability-with-layered-architecture-25e74ba86430)

It could also facilitate unit tests for the business logic, since by putting it here you would have to mount the component and all its dependencies in order to test this logic.

Does it make sense?

Maybe, this only affects the UI, but it is a pretty important piece of UI on the transaction data.
But I don't think it's in the scope of this PR, since we should think of how to structure the UI and business logic tests with electron.

That's ok for me. But if you think this is important, we should create an issue or KTLO, so we don't forget.

fix: remove registered tokens when changing networks

03a99b6

r4mmer self-assigned this Aug 31, 2023

r4mmer requested a review from pedroferreira1 as a code owner August 31, 2023 16:04

r4mmer requested review from luislhl and alexruzenhack and removed request for pedroferreira1 August 31, 2023 16:06

luislhl reviewed Sep 4, 2023

View reviewed changes

alexruzenhack approved these changes Sep 5, 2023

View reviewed changes

r4mmer added 5 commits September 8, 2023 14:40

chore: wallet-lib bump

b3ec6b9

fix: remove testnet suffix when configuring the network

a6b001d

fix: tx balance not being calculated correctly

b92658f

chore: get wallet words from util

6cbee5a

fix: unexpected state on error

2d32095

r4mmer commented Sep 13, 2023

View reviewed changes

chore: use wallet-service facade method to update network

e61ff75

r4mmer commented Sep 13, 2023

View reviewed changes

r4mmer requested a review from alexruzenhack September 13, 2023 14:04

chore: change callback to async so we can use await

50235ca

r4mmer requested a review from luislhl September 13, 2023 14:27

alexruzenhack approved these changes Sep 13, 2023

View reviewed changes

chore: stop processing the saga on error

6c7fc04

luislhl reviewed Sep 13, 2023

View reviewed changes

src/reducers/index.js Outdated Show resolved Hide resolved

luislhl reviewed Sep 13, 2023

View reviewed changes

chore: use network name without suffixes

5480cd8

r4mmer mentioned this pull request Sep 14, 2023

Add cleanRegisteredTokens option to wallet facade's stop HathorNetwork/hathor-wallet-lib#575

Open

luislhl approved these changes Sep 14, 2023

View reviewed changes

r4mmer merged commit cef3df7 into dev Sep 14, 2023

r4mmer deleted the fix/change-server branch September 14, 2023 17:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: change server #411

fix: change server #411

r4mmer commented Aug 31, 2023

codecov bot commented Aug 31, 2023 •

edited

Loading

luislhl Sep 4, 2023

r4mmer Sep 13, 2023

luislhl Sep 13, 2023

r4mmer Sep 14, 2023

r4mmer Sep 13, 2023

r4mmer Sep 13, 2023

luislhl Sep 13, 2023

r4mmer Sep 14, 2023

luislhl Sep 14, 2023

luislhl Sep 13, 2023 •

edited

Loading

r4mmer Sep 14, 2023

luislhl Sep 14, 2023

fix: change server #411

fix: change server #411

Conversation

r4mmer commented Aug 31, 2023

Acceptance Criteria

Security Checklist

codecov bot commented Aug 31, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luislhl Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 31, 2023 •

edited

Loading

luislhl Sep 13, 2023 •

edited

Loading