New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix multiple bugs with fees & refactor #474

Merged
merged 8 commits into from Dec 31, 2017

Conversation

Projects
None yet
3 participants
@Nasicus
Contributor

Nasicus commented Dec 18, 2017

So I decided to tackle this guy! Took me quite a while to figure out the main problem :)

Main problem was:

In the desktop we already used the method getFees to get fees from the network, however we then used arkjs to create our transactions. arkjs always uses the default values for all fees of all transaction types (see here for an example: https://github.com/ArkEcosystem/ark-js/blob/0b8d460a54657b24703e0b7fe59cf2067e00491b/lib/transactions/vote.js#L30 ) and we did never overwrite them.

So basically if a network had different fees for a transaction type, we ignored it and just used the default value...

Secon problem:
When clicking "Send all" in the send dialog, we used a hardcoded fee type, instead of the one from the network (however this is just a ui bug).

NOTE:
I will probably add a third commit to refactor this createTransaction method, which is imho, horribly big :P

Fixes: #453

@perryhoffman

This comment has been minimized.

Show comment
Hide comment
@perryhoffman

perryhoffman Dec 19, 2017

Contributor

"I will probably add a third commit to refactor this sendTransaction method, which is imho, horribly big" I second this 👍 . It will make it a lot easier to test once it is refactored as well!

Contributor

perryhoffman commented Dec 19, 2017

"I will probably add a third commit to refactor this sendTransaction method, which is imho, horribly big" I second this 👍 . It will make it a lot easier to test once it is refactored as well!

@Nasicus

This comment has been minimized.

Show comment
Hide comment
@Nasicus

Nasicus Dec 19, 2017

Contributor

Ok I pushed my refactor of this method...let me quote the commit message what I did:

  • remove duplicated code (we had 4 times practically the same code)
  • split createTransaction method into 4 public methods, which makes it clear from outside what happens
  • fix: potential wrong ark fees in toast messages (within create transaction)
  • fix: hardcoded token name "Ark" within toast messages (within create transaction)

I highly recommend reviewing this PR commit by commit to the reviewer - since the last commit looks like a lot happened even though it didn't.

By the way, while refactoring I also detected an "anomaly" which I'm not sure is a bug or not:
Only when creating a vote transaction we do the following on the transaction (in case of using the ledger):
transaction.recipientId = config.fromAddress
On all other transaction types we do NOT do this.
Is it correct, that this is only done here?

@perryhoffman and reviewer
What do you think about the refactor? I'm not sure if I'm satisfied... do you have any suggestions / better ideas?

Contributor

Nasicus commented Dec 19, 2017

Ok I pushed my refactor of this method...let me quote the commit message what I did:

  • remove duplicated code (we had 4 times practically the same code)
  • split createTransaction method into 4 public methods, which makes it clear from outside what happens
  • fix: potential wrong ark fees in toast messages (within create transaction)
  • fix: hardcoded token name "Ark" within toast messages (within create transaction)

I highly recommend reviewing this PR commit by commit to the reviewer - since the last commit looks like a lot happened even though it didn't.

By the way, while refactoring I also detected an "anomaly" which I'm not sure is a bug or not:
Only when creating a vote transaction we do the following on the transaction (in case of using the ledger):
transaction.recipientId = config.fromAddress
On all other transaction types we do NOT do this.
Is it correct, that this is only done here?

@perryhoffman and reviewer
What do you think about the refactor? I'm not sure if I'm satisfied... do you have any suggestions / better ideas?

@j-a-m-l j-a-m-l self-requested a review Dec 19, 2017

@j-a-m-l

j-a-m-l requested changes Dec 19, 2017 edited

I like it, but I'd add some tests, to ensure that it isn't breaking anything before starting the manual review.

One possible improvement, would be creating a new service, something like transactionBuilder, transactionCreator, to move code outside AccountCtrl.

About the "anomaly" I'll try to tell you later, after having the new changes.

@Nasicus

This comment has been minimized.

Show comment
Hide comment
@Nasicus

Nasicus Dec 19, 2017

Contributor

@j-a-m-l
What kind of tests do you expect? Testing the createTransactions methods? And if so do I need to mock the ledgerService, storageService and so on? As far as I can see there is no "base" yet for testing this service? (I never wrote tests in angular 1 yet)
@perryhoffman
Did you already start writing tests for this method? (just because you mentioned it in your comment)

Edit:

I tried writing unit tests for the accountService...however I have no working base at all (and I also think there isn't any).
And I think the problem is not even that my "injects" of the other servies are not working.... the problem is, that all require (for example in account.service.js itself (which I obviously don't want to mock): var ark = require('../node_modules/arkjs') ) do not work and I get errros like this when running karma tests: Error: Cannot find module 'E:devgitark-desktop est/../client/app\../node_modules/arkjs'
Here you can find my current test class: https://gist.github.com/Nasicus/627e7fd05dd58121336cdf6e14927ab2

Edit2:

Note that I also get this bug for existing tests, for example:

Electron 1.7.9 (Node 7.9.0) AccountController "before each" hook for "returns the user accounts only" FAILED
        Error: Cannot find module 'E:devgitark-desktop  est/../client/app\../../package.json'
            at Module._resolveFilename (module.js:470:15)
            at Function.Module._resolveFilename (E:\dev\git\ark-desktop\node_modules\electron\dist\resources\electron.asar\common\reset-search-paths.js:35:12)
            at Function.Module._load (module.js:418:25)
            at Module.require (module.js:498:17)
            at require (internal/module.js:20:19)
            at window.require (E:/dev/git/ark-desktop/client/app/src/init.js:9:2001)
            at window.require (E:/dev/git/ark-desktop/client/app/src/accounts/account.service.js:9:2001)
            at window.require (E:/dev/git/ark-desktop/client/app/src/accounts/account.controller.js:9:2485)
            at window.require (E:/dev/git/ark-desktop/client/app/src/components/account/account-card.controller.js:9:2001)
            at window.require (E:/dev/git/ark-desktop/client/app/src/components/account/delegate-tab.controller.js:9:2001)
        Error: Declaration Location
            at window.inject.angular.mock.inject (E:/dev/git/ark-desktop/node_modules/angular-mocks/angular-mocks.js:3145:25)
            at Context.<anonymous> (accounts/account.controller.js:152:7)

Can anyone else confirm if tests work on his machine?
I'm using Windows 10 btw.

Contributor

Nasicus commented Dec 19, 2017

@j-a-m-l
What kind of tests do you expect? Testing the createTransactions methods? And if so do I need to mock the ledgerService, storageService and so on? As far as I can see there is no "base" yet for testing this service? (I never wrote tests in angular 1 yet)
@perryhoffman
Did you already start writing tests for this method? (just because you mentioned it in your comment)

Edit:

I tried writing unit tests for the accountService...however I have no working base at all (and I also think there isn't any).
And I think the problem is not even that my "injects" of the other servies are not working.... the problem is, that all require (for example in account.service.js itself (which I obviously don't want to mock): var ark = require('../node_modules/arkjs') ) do not work and I get errros like this when running karma tests: Error: Cannot find module 'E:devgitark-desktop est/../client/app\../node_modules/arkjs'
Here you can find my current test class: https://gist.github.com/Nasicus/627e7fd05dd58121336cdf6e14927ab2

Edit2:

Note that I also get this bug for existing tests, for example:

Electron 1.7.9 (Node 7.9.0) AccountController "before each" hook for "returns the user accounts only" FAILED
        Error: Cannot find module 'E:devgitark-desktop  est/../client/app\../../package.json'
            at Module._resolveFilename (module.js:470:15)
            at Function.Module._resolveFilename (E:\dev\git\ark-desktop\node_modules\electron\dist\resources\electron.asar\common\reset-search-paths.js:35:12)
            at Function.Module._load (module.js:418:25)
            at Module.require (module.js:498:17)
            at require (internal/module.js:20:19)
            at window.require (E:/dev/git/ark-desktop/client/app/src/init.js:9:2001)
            at window.require (E:/dev/git/ark-desktop/client/app/src/accounts/account.service.js:9:2001)
            at window.require (E:/dev/git/ark-desktop/client/app/src/accounts/account.controller.js:9:2485)
            at window.require (E:/dev/git/ark-desktop/client/app/src/components/account/account-card.controller.js:9:2001)
            at window.require (E:/dev/git/ark-desktop/client/app/src/components/account/delegate-tab.controller.js:9:2001)
        Error: Declaration Location
            at window.inject.angular.mock.inject (E:/dev/git/ark-desktop/node_modules/angular-mocks/angular-mocks.js:3145:25)
            at Context.<anonymous> (accounts/account.controller.js:152:7)

Can anyone else confirm if tests work on his machine?
I'm using Windows 10 btw.

@perryhoffman

This comment has been minimized.

Show comment
Hide comment
@perryhoffman

perryhoffman Dec 19, 2017

Contributor

@Nasicus I haven't written any tests for the service methods yet. I was hoping they would be refactored first, it would make writing asserts much more manageable!

You're right though, there currently isn't a base spec file for the accounts service. I just gave it a shot on my local. It seems to work fine, with the addition of 1 mock you were missing $provide.value('ARKTOSHI_UNIT', Math.pow(10, 8)). (Ref: https://gist.github.com/perryhoffman/3f00c6b5c2ed8d4741a2b84899add0f8)

Can you double check that you have npm installed properly? And that the arkjs package exists inside the client client/node_modules directory (not the root node_modules dir)?

Contributor

perryhoffman commented Dec 19, 2017

@Nasicus I haven't written any tests for the service methods yet. I was hoping they would be refactored first, it would make writing asserts much more manageable!

You're right though, there currently isn't a base spec file for the accounts service. I just gave it a shot on my local. It seems to work fine, with the addition of 1 mock you were missing $provide.value('ARKTOSHI_UNIT', Math.pow(10, 8)). (Ref: https://gist.github.com/perryhoffman/3f00c6b5c2ed8d4741a2b84899add0f8)

Can you double check that you have npm installed properly? And that the arkjs package exists inside the client client/node_modules directory (not the root node_modules dir)?

@j-a-m-l

This comment has been minimized.

Show comment
Hide comment
@j-a-m-l

j-a-m-l Dec 20, 2017

Member

@Nasicus I've tried the last commit (with npm test at the root folder of the project, in Ubuntu) and only 1 test has failed.

About which kind of tests would I expect: I'd move those methods outside the AccountController, as a service, and then, I'd test simple things initially.

If you prefer, just create the service and fix the one that is failing, and I'll add more tests myself.

Member

j-a-m-l commented Dec 20, 2017

@Nasicus I've tried the last commit (with npm test at the root folder of the project, in Ubuntu) and only 1 test has failed.

About which kind of tests would I expect: I'd move those methods outside the AccountController, as a service, and then, I'd test simple things initially.

If you prefer, just create the service and fix the one that is failing, and I'll add more tests myself.

@Nasicus

This comment has been minimized.

Show comment
Hide comment
@Nasicus

Nasicus Dec 21, 2017

Contributor

So after what feels like an enternity to set up the "base ground" for testing I'm finally done for the moment ;)

@j-a-m-l
I extracted the methods into an own service and added "a few" ( ;) ) unit tests.

Check it out and let me know if you need more.
I wrote this tests which immediately came to my mind.

I will do some more tests (manual on darknet) and rebase my branch this evening.

Contributor

Nasicus commented Dec 21, 2017

So after what feels like an enternity to set up the "base ground" for testing I'm finally done for the moment ;)

@j-a-m-l
I extracted the methods into an own service and added "a few" ( ;) ) unit tests.

Check it out and let me know if you need more.
I wrote this tests which immediately came to my mind.

I will do some more tests (manual on darknet) and rebase my branch this evening.

@Nasicus

This comment has been minimized.

Show comment
Hide comment
@Nasicus

Nasicus Dec 21, 2017

Contributor

Phew... ok:

  • rebased
  • extracted logic of arktoshiToArk into seperate service (just couldn't let them be in accountService, they don't belong there)
    • note: there are many other places in code where we can now use this service, but I didn't want to have to many changes, so I only changed the files where absolutely necessary. Just tell me if you want me to replace this in this PR or in a new one :) (and what you think of it anywy :D )
  • also added tests for this new service
  • manually tested send, vote, createSecondPassPhrase & create delegate on darknet
Contributor

Nasicus commented Dec 21, 2017

Phew... ok:

  • rebased
  • extracted logic of arktoshiToArk into seperate service (just couldn't let them be in accountService, they don't belong there)
    • note: there are many other places in code where we can now use this service, but I didn't want to have to many changes, so I only changed the files where absolutely necessary. Just tell me if you want me to replace this in this PR or in a new one :) (and what you think of it anywy :D )
  • also added tests for this new service
  • manually tested send, vote, createSecondPassPhrase & create delegate on darknet

@Nasicus Nasicus changed the title from Fix two bugs with fees & refactor to Fix multiple bugs with fees & refactor Dec 30, 2017

Nasicus added some commits Dec 18, 2017

fix: wrong fees were potentially used for all transaction types
- happened as soon as a network had not the standard fee anymore for any transaction type
fix: hardcoded fee was used to calculate the balance when "Send all" …
…is clicked

- also make the "getFees" cacheable, but for transactions still get the fee directly from the network
refactor create transaction method (minor fixes)
- remove duplicated code (we had 4 times practically the same code)
- split createTransaction method into 4 public methods, which makes it clear from outside what happens
- fix: potential wrong ark fees in toast messages (within create transaction)
- fix: hardcoded token name "Ark" within toast messages (within create transaction)
extract arktoshiToArk and numberToFixed in utilityService, add unit t…
…ests

- also add missing entry in index.html for TransactionBuilderService
- add one more expect in unit tests to ensure the correct token name is in the error message
@@ -971,6 +801,9 @@
fetchAccountAndForget: fetchAccountAndForget,
// return a copy of the object, so the original can't be changed
defaultFees: JSON.parse(JSON.stringify(self.defaultFees)),

This comment has been minimized.

@j-a-m-l

j-a-m-l Dec 31, 2017

Member

Probably a better option is freezing the object (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/freeze). It's something that we should start using on more places.

@j-a-m-l

j-a-m-l Dec 31, 2017

Member

Probably a better option is freezing the object (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/freeze). It's something that we should start using on more places.

@j-a-m-l j-a-m-l merged commit 38f0af8 into ArkEcosystem:master Dec 31, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Nasicus Nasicus deleted the Nasicus:bug/wrong-fees branch Dec 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment