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

refactor: Use @parity/* 4.0.0 ; refactor sendStore post/postRaw #394

Closed
wants to merge 4 commits into from

Conversation

axelchalon
Copy link
Contributor

@axelchalon axelchalon commented Jan 24, 2019

openethereum/js-libs#93

  • Update dependencies to @parity/* 4.0.0
  • Update/refactor sendStore (post/postRaw)
  • Remove ws token (secure ui token); not needed anymore

You can test:

  • Sending ETH from node account
  • Sending ETH from signer account
  • Sending erc20 from node account
  • Sending erc20 from signer account

and make sure it all works (works for me)

- Update dependencies to @parity/* 4.0.0
- Update/refactor sendStore (post/postRaw)
- Remove ws token (secure ui token); not needed anymore
@axelchalon
Copy link
Contributor Author

axelchalon commented Jan 24, 2019

Now that we don't need a secure token from parity, we don't need to run parity signer new-token, so we don't need to know the path to Parity. i.e. as long as we can connect to a Parity instance, we're good to go.

We should probably rewrite
https://github.com/paritytech/fether/blob/1fd1b98819ab11daa9da9ab7a3d6a6df3c988bd2/packages/fether-electron/src/main/index.js#L51-L60

Parity path is now only useful to be able to launch Parity.

The new logic I'd suggest (on startup):

  • Try to connect to Parity
  • If it's not running, try to launch Parity if it's in the path/known locations
  • If it's not in the path, download and launch

@amaury1093
Copy link
Collaborator

Doesn't removing the token introduce a security issue?

@axelchalon
Copy link
Contributor Author

axelchalon commented Jan 24, 2019

How so? Do you mean that ideally we should launch parity without enabling the personal api, but that it should still be available for ws connections with secure token?

@amaury1093
Copy link
Collaborator

Yes, I thought that was the purpose of the secure token: if you have the token, you have access to signer_ and personal_. I'm maybe wrong though.

The simple attack vector I can think of is, if we're on the same network, I can send a curl request to your node on the personal_ endpoint.

@axelchalon
Copy link
Contributor Author

Yes, makes sense.

Currently the list of APIs enabled for WS connections made with a secure token is hardcoded (i.e. ignores run flags) and includes every api except for personal. I guess we should change that so that secure WS connections have access to all apis.

@axelchalon
Copy link
Contributor Author

Tracking openethereum/parity-ethereum#10246 (enable personal_ in secure ws connection, regardless of enabled ws apis in launch flags)

However, if we continue launching Parity Ethereum with default flags, after this PR users will only be able to use Fether with the most recent (upcoming) Parity Ethereum release. This is a problem e.g. for users who be updating Fether, as Parity Ethereum won't get downloaded again (hello #204). So I think that, at least for a certain amount of time we should launch Fether with --ws-apis=all for compatibility with the current versions (unless the error message reads "Please update to the latest Parity Ethereum or launch Parity Ethereum with the --ws-apis=all flags" (however there's no straightforward way for Fether users to redownload Parity Ethereum..))

If you have a simpler idea feel free to share...

@axelchalon axelchalon changed the title refactor: Use @parity/* 4.0.0 ; remove signer_ rpc & ws sec token refactor: Use @parity/* 4.0.0 ; refactor sendStore post/postRaw Jan 25, 2019
@axelchalon
Copy link
Contributor Author

axelchalon commented Jan 28, 2019

This PR will require Parity Ethereum >=v2.4.0

Before merging, requires:

  • Parity Ethereum personal_ PR being merged [DONE] and Parity Ethereum v2.4.0 being released
  • The PR taking care of bundling Parity Ethereum into Fether to be merged; that PR will also take care of specifying the versions of Parity Ethereum compatible with the current release of Fether (useful in case an instance of Parity Ethereum is already running)
  • Specifying that we require Parity Ethereum 2.4.x using the tooling of the above-mentioned PR

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.

lgtm!

@axelchalon axelchalon closed this Mar 13, 2019
@axelchalon
Copy link
Contributor Author

axelchalon commented Mar 13, 2019

merged into #458

@amaury1093 amaury1093 deleted the ac-upd-post branch March 13, 2019 16:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants