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

Display warning for gas price #872

Merged
merged 2 commits into from Sep 19, 2018
Merged

Display warning for gas price #872

merged 2 commits into from Sep 19, 2018

Conversation

alaibe
Copy link
Contributor

@alaibe alaibe commented Sep 19, 2018

Overview

TL;DR

Display a warning when gas price won't be display

Cool Spaceship Picture

star-destroyer_ab6b94bb

@alaibe alaibe requested a review from a team September 19, 2018 11:15
cb();
});
}

showNodeHttpWarning() {
if (this.options.node.startsWith('http')) {
this.engine.logger.warn("You are using http to connect to the node, as a result the gas details won't be correct");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we maybe add a possible solution for the user? For example,

You are using http to connect to the node, as a result the gas details won't be correct. For correct gas details reporting, please use a websockets connection to your node.

What do you think of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much better indeed, I updated the text

@michaelsbradleyjr
Copy link
Contributor

When running embark test --node embark, if the node is configured to provide a websockets endpoint, should the connection default to using that instead of http?

@alaibe
Copy link
Contributor Author

alaibe commented Sep 19, 2018

@michaelsbradleyjr it will default to use web socket and no warning will be printed in that case

@michaelsbradleyjr
Copy link
Contributor

When running embark test --node embark in a fresh embark_demo from the next branch (embark run is up in another terminal) it's defaulting to http.

@alaibe
Copy link
Contributor Author

alaibe commented Sep 19, 2018

This is because the demo is configure to use http and not websocket.
https://github.com/embark-framework/embark/blob/develop/templates/demo/config/contracts.js#L6-L8

using node embark => behind the hood it means use IPC. and the main process is using http not ws

Does that make sense?

@michaelsbradleyjr
Copy link
Contributor

michaelsbradleyjr commented Sep 19, 2018

Makes perfect sense, thanks. I guess what I'm wondering is if in the case of embark test --node embark we should detect that ws provider is available and use that as first preference so that gas details will be correctly reported by default. I'm just thinking about what would make the best experience for the user, but I definitely don't see it as a major problem.

@alaibe
Copy link
Contributor Author

alaibe commented Sep 19, 2018

I guess what you are suggesting is almost a new option: auto where we check what nodes are available based on the config.

@iurimatias what do you think?

@michaelsbradleyjr
Copy link
Contributor

michaelsbradleyjr commented Sep 19, 2018

--node auto might be okay. I'm thinking of it more like this: in our demo dapp, for example, embark test --node embark will give a warning about gas (per this PR). There is no additional flag such that --node embark won't result in a warning, only option is to switch to --node ws://localhost:8546 (or :8556). It just seems a little awkward that there's not a straightforward way for --node embark to be fully functional (correct gas details).

@alaibe
Copy link
Contributor Author

alaibe commented Sep 19, 2018

Gotcha (I think). There is another way. Change the demo config to use websocket for contract deployment:
https://github.com/embark-framework/embark/blob/develop/templates/demo/config/contracts.js#L6-L8

That way there will be no warning

@michaelsbradleyjr
Copy link
Contributor

That would work. What do you think @iurimatias, should we update the templates?

@iurimatias
Copy link
Collaborator

I agree it should be changed in the template, I thought it was already the default in fact

@iurimatias iurimatias merged commit b817c0f into next Sep 19, 2018
@iurimatias iurimatias deleted the feature/warning-for-gas branch September 22, 2018 13:24
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

4 participants