Skip to content
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

Wallet query for all stakepools #118

Merged
merged 4 commits into from Jan 23, 2020

Conversation

jpcapurro-atixlabs
Copy link
Contributor

@jpcapurro-atixlabs jpcapurro-atixlabs commented Jan 21, 2020

developed against jormungandr 0.8.4

Copy link
Contributor

@dmernies-iohk dmernies-iohk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes are understood and the unit tests work well. There are delays to open the wallet with yarn dev. You must specify the version of jorgumandr which the wallet should be tested, maybe the delays are due to different versions, currently I tested with jormungandr 0.8.0-rc10

@@ -0,0 +1,92 @@
// @flow
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thiks test looks and executes good but there is an error on SendTransaction when executing yarn test

    console.error app/components/SendTransaction.js:36
      couldnt send funds:  Error: Error
          at Object.rejects (/home/atixlabs/src/iohk/js-chain-libs/examples/wallet/node_modules/sinon/lib/sinon/default-behaviors.js:193:22)
          at Object.proto.<computed> [as rejects] (/home/atixlabs/src/iohk/js-chain-libs/examples/wallet/node_modules/sinon/lib/sinon/behavior.js:238:12)
          at Function.rejects (/home/atixlabs/src/iohk/js-chain-libs/examples/wallet/node_modules/sinon/lib/sinon/behavior.js:231:46)
          at Object.<anonymous> (/home/atixlabs/src/iohk/js-chain-libs/examples/wallet/test/components/SendTransaction.spec.js:118:19)
          at Object.asyncJestLifecycle (/home/atixlabs/src/iohk/js-chain-libs/examples/wallet/node_modules/jest-jasmine2/build/jasmineAsyncInstall.js:53:37)
          at /home/atixlabs/src/iohk/js-chain-libs/examples/wallet/node_modules/jest-jasmine2/build/queueRunner.js:43:12
          at new Promise (<anonymous>)
          at mapper (/home/atixlabs/src/iohk/js-chain-libs/examples/wallet/node_modules/jest-jasmine2/build/queueRunner.js:26:19)
          at /home/atixlabs/src/iohk/js-chain-libs/examples/wallet/node_modules/jest-jasmine2/build/queueRunner.js:73:41

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the expected error that we talked about in a previous issue


export function setStakePools(): (
dispatch: Dispatch<SetStakePoolsAction>
) => Promise<SetStakePoolsAction> {
) => Promise<mixed> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the mixed type returned?
Also the function that is invoked in setStakePoolsThunk is getStakePools (), should it be called setStakePools?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the mixed type tells the type system that anything could be returned, because currently it doesn't return anything useful.

Trying to use a mixed type will always fail, so the type system will tell me if I try to use the return vale. Currently we only care about wether the promise resolves or rejects.

}
throw error;
});
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the application is run with yarn dev or yarn start, after removing node_modules, yarn.lock and npm install, it takes a long time to open the instance. Can this builder influence the opening of the wallet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you try to do the same on a different revision?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

going from zero to 'app running' involves three stages:

  • installing the dependencies: this is done via yarn install and takes a lot of time because the actual dependency tree for this and every node proyect is huge. This has been widely memed about: https://www.improgrammer.net/npm-package-delivery/

  • building the renderer and main 'processes': this is done via yarn build, and consists of transpiling all our source code and assets into a 'bundle' that a browser can actually run. Webpack takes care of this, and no changes were introduces in this PR that might increase transpilation time.

  • running the app: this consists of firing up the browser and actually running our (transpiled) code.
    Having the builder only adds one syncronous function call to inject the dependencies, with nothing taking up particularly long-running. That's why I'm pretty skeptical of the builder increasing the load time. I'd however be pretty curious to try to replicate any benchmark 🙃.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think It's Ok. worked with node 10.

@dmernies-iohk
Copy link
Contributor

It's Ok to merge

@jpcapurro-atixlabs jpcapurro-atixlabs merged commit 59df7da into master Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants