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

feat: add versionInfo$ #205

Merged
merged 4 commits into from Mar 12, 2019
Merged

feat: add versionInfo$ #205

merged 4 commits into from Mar 12, 2019

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

@@ -36,7 +36,7 @@ export const switchMapPromise = <T,U>(promise: () => Promise<U>) => (
)
);
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.

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
@@ -40,5 +40,8 @@
"typedoc": "^0.14.2",
"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

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.

None yet

2 participants