Skip to content
This repository was archived by the owner on May 24, 2022. It is now read-only.

feat: add versionInfo$#205

Merged
amaury1093 merged 4 commits intomasterfrom
ac-versioninfo
Mar 12, 2019
Merged

feat: add versionInfo$#205
amaury1093 merged 4 commits intomasterfrom
ac-versioninfo

Conversation

@axelchalon
Copy link
Contributor

@axelchalon axelchalon commented Mar 5, 2019

Some changes required for openethereum/fether#204

  • Add versionInfo$
  • Rejected/errored RPCs now make the observable emit an error if options.emitErrors === true. This is needed to check the version of the running instance of Parity (part of Bundle Parity Ethereum in Fether's binary fether#204): if the RPC parity_versionInfo fails, we know that we're running Parity Ethereum <v2.4.1 (in fether-react)
  • Add parityPath to runParity and signerNewToken, which lets us run those commands on the bundled Parity binary

Copy link
Collaborator

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

Looks good overall, just not 100% sure if we should error the Observable

);
console.groupEnd();
return empty();
return throwError(err) as Observable<U>;
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 actually remove the catchError block altogether? Also, the function comment should be modified.


On another note: The reason I initially wanted to return empty() was because of e.g. balanceOf$(), if one of the eth_getBalance calls fails for whatever reason, then the whole Observable errors and stops. In fether, I had some occasions where transactionCount$ failed once or twice because of the light node, but it retried on new head, so the overall behavior was okay. I just don't want transactionCount$ to stop completely if one of the eth_getTransactionCount fails.

@axelchalon axelchalon requested a review from amaury1093 March 11, 2019 16:38
Copy link
Collaborator

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

This sounds good to me! Will merge once tests pass

@amaury1093 amaury1093 self-requested a review March 12, 2019 09:40
package.json Outdated
"typedoc-plugin-markdown": "^1.1.13",
"typescript": "^3.1.6"
},
"dependencies": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this shouldn't be here

@amaury1093 amaury1093 merged commit e18d839 into master Mar 12, 2019
@amaury1093 amaury1093 deleted the ac-versioninfo branch March 12, 2019 17:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants