-
Notifications
You must be signed in to change notification settings - Fork 22
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
Added a backported websocket provider implementaton for web3 0.x #255
Conversation
Pull Request Test Coverage Report for Build 5419
💛 - Coveralls |
@@ -31,7 +31,7 @@ class Main extends React.Component<DispatchProp<any> & RouteComponentProps<any>> | |||
|
|||
public onNetworkUpdated = async (): Promise<void> => { | |||
const civil = getCivil(); | |||
if (civil.network) { | |||
if (civil.network === "4") { | |||
this.props.dispatch!(setNetwork(civil.network)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to set the network even if it's not rinkeby so that we properly render the error ("switch to rinkeby") bar
this.unbscribeEthereum(); | ||
} | ||
|
||
public createEthereumSubscription(civil?: Civil): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this stuff is good but it should all happen outside of the component and be stored in redux, like how we store network/account in redux currently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nickreynolds There is no redux in the components package and it's used in multiple places, not only the dapp. This is the most backwards-compatible way I've found. If we get eg a standard of how we should write react components and refactor all our front-ends then sure
cc @dankins we really need one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well you would just pass this data in to the component as a prop and the DApp would get it from its redux
clazz.new( | ||
{{> params inputs=contract.inputs}} | ||
{{> params inputs=ctor.inputs}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure why this changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good except the part where we get network/account in the component instead of from redux and there's a few debugging comments left in.
63e2fd0
to
a711444
Compare
Minor truffle versions upgrade not only features, but also compiler versions, which have new imcompatible changes
a711444
to
200b672
Compare
Caveat - The provider never closes and keeps the Node process forever running. This is consistent with the upstream implementation