fund account via ShapeShift #2099

jacogr · 2016-09-15T08:09:27Z

implements the outstanding Fund Account functionality. Demo of the functionality -

https://youtu.be/1i37yMa3NCc (EDIT: Updated the UI slightly so we don't need to come back to it immediately - https://youtu.be/BIS47mYRxlk)

Requires/includes https://github.com/ethcore/parity/pull/2088 which have been extended slightly to have event subscribe functionality as we have in the other APIs. (Contract, Core)

Future work: (EDIT: addressed in the last commit, updated video added above)

~~UI could be prettier & much more graphical and snazzy :(~~
~~there is an initial delay on clicking shift which is not a perfect UX (some initial busy feedback would be good)~~

* js-3rdparty-shapeshift: call is not needed within closure define functions explicitly withing closure fix tests with new structure, proper naming es6-ify library & flatten GPL header initial import before tweaking starts

parity-cla-bot · 2016-09-15T08:09:28Z

It looks like @jacogr signed our Contributor License Agreement. 👍

Many thanks,

Ethcore CLA Bot

derhuerst

Tiny remarks:

the modal window is not scrollable on small screens
at the 2. step, the code should ideally be easily copied to clipboard
for the "fund account" button that opens the modal, i'd like to have a different icon, e.g. https://design.google.com/icons/#ic_attach_money

derhuerst · 2016-09-15T15:39:42Z

js/src/modals/FundAccount/AwaitingDepositStep/awaitingDepositStep.js

+    return (
+      <div className={ styles.center }>
+        <div className={ styles.info }>
+          <a href='https://shapeshift.io' target='_blank'>ShapeShift.io</a> is awaiting a { typeSymbol } deposit. Send the funds from you { typeSymbol } network client to -


typo. it should be "fund from your … network".

ty, will address.

jacogr · 2016-09-15T15:53:30Z

Valid, not going to be fixed in this PR, but logging them:

Yeap, none of the modals are currently scrollable. Should be fixed in ui/Modal.
We have a react-copy-to-clipboard in there, I guess it is actually wider than that - we also have the account phrase we should be copy-able. Should actually go through all do all of them in the same place.

Valid, will fix:

think a different icon is in order, in this case though it is not real money and we should rather use a money symbol for when we have real $ -> ETH integration (Mist added something like that in the last release via Coinbase). Any other ideas of icons in this case? @derhuerst @ngotchac ?

[EDIT: just going to use their btn icon & name, self-explanatory]

rphmeier · 2016-09-15T16:08:19Z

js/src/modals/FundAccount/CompletedStep/completedStep.js

+          <Value amount={ incomingCoin } symbol={ incomingType } /> => <Value amount={ outgoingCoin } symbol={ outgoingType } />
+        </div>
+        <div className={ styles.info }>
+          The funds will reflect in your Parity account shortly.


typo? Should be "The change in funds will be reflected ..."

derhuerst · 2016-09-15T15:50:19Z

js/src/3rdparty/shapeshift/rpc.js

+function call (method, options) {
+  return fetch(`${ENDPOINT}/${method}`, options)
+    .then((response) => {
+      if (response.status !== 200) {


I think checking for 200 <= status && status < 300 is safer, as the HTTP RFC defines 2xx as "client's request was successfully received, understood, and accepted.".

Edit: Check for response.ok.

response.ok :)

derhuerst · 2016-09-15T15:56:10Z

js/src/3rdparty/shapeshift/rpc.js

+  return fetch(`${ENDPOINT}/${method}`, options)
+    .then((response) => {
+      if (response.status !== 200) {
+        throw { code: response.status, message: response.statusText }; // eslint-disable-line


As people often check for err instanceof Error, i'd consider using new Error(…) here.

Agreed. Actually needs to be done to align with the other APIs as well.

derhuerst · 2016-09-15T16:01:01Z

js/src/3rdparty/shapeshift/shapeshift.js

+            return;
+
+          case 'failed':
+            subscription.callback({


As above – using new Error(…) might be useful, because people tend to do err instanceof Error or err.toString().

Only issue here is that there are 2 types of errors - recoverable and non-recoverable. The fatal flag indicates that. So not 100% sure how to handle that.

derhuerst · 2016-09-15T16:01:42Z

js/src/3rdparty/shapeshift/shapeshift.js

+  }
+
+  function _pollStatus () {
+    subscriptions.map(_getSubscriptionStatus);


should be .forEach.

Good catch, fixed.

derhuerst · 2016-09-15T16:10:10Z

js/src/modals/FundAccount/AwaitingDepositStep/awaitingDepositStep.js

+        </div>
+      );
+    }
+    return (


In AwaitingDepositStep, AwaitingExchangeStep, CompletedStep, ErrorStep and OptionsStep, there's a lot of duplicate/boilerplatisch code. It's a lot of stuff to read through for not soo much complexity being handled; Maybe we can rduce that a little. :)

Also, but this is a matter of taste, having separate directories for these steps is bloat.

Each component in their own directory. We've been following that through the whole codebase.

derhuerst · 2016-09-15T16:15:55Z

js/src/modals/Shapeshift/Value/value.js

+
+    return (
+      <div className={ styles.body }>
+        <span>{ value }</span><small>{ symbol || 'ΞTH' }</small>


<span> is an inline element. As of HTML5, <small> has a semantical meaning and should not be used for stylistic purposes.

Should fix it in a fixes PR - I actually think I started adopting that after seeing it in the Signer. Agreed in principle though, but needs to be done properly. (May be worth it to pull in this component there then and whereever we do this type of thing)

derhuerst · 2016-09-15T16:18:38Z

js/src/modals/Shapeshift/shapeshift.js

+  }
+
+  state = {
+    stage: 0,


I think strings as values of stage would be both more robust & easier to reason about.

I think in this case yes, since there is not a lot of jumping. Just keeping it consistent though.

derhuerst · 2016-09-15T16:21:07Z

js/src/modals/Shapeshift/shapeshift.js

+  return {};
+}
+
+function mapDispatchToProps (dispatch) {


I think using Redux just for this isn't worth it, given that Redux adds complexity and makes it harder to follow the flow of data. I'd either go all in and move this component's state to Redux, or remove it.

Bringing this whole Shapeshift logic to Redux totally makes sense to me, as it is easily testable, (in doubt) more robust and modelled independently from the UI.

The Redux state is just there for the newError action. There is no internal state in the Redux store.

Exactly, I consider this as "not worth" the Redux complexity. I propose to pull the stuff that is in state right now into Redux.

If this was a component like a dapp, self-contained, yes. I honestly don't want to be coding for the next 2 days just to do that complexity and conversion where the state is only applicable to the single component.

So no, I'd rather get features out than coding for the sake of coding where in the end I won't be able to follow the actual code. I think Redux is great where I need global state, don't need it here. (newError is global)

If this was a component like a dapp, self-contained, yes. I honestly don't want to be coding for the next 2 days just to do that complexity and conversion where the state is only applicable to the single component.

So no, I'd rather get features out than coding for the sake of coding where in the end I won't be able to follow the actual code. I think Redux is great where I need global state, don't need it here. (newError is global)

Fair enough.

Often state is more global than one immeadiatly thinks. There's more parts of the app that should pay attention to each other than we lazy developers like to think of.

derhuerst · 2016-09-15T16:25:26Z

js/src/ui/IdentityIcon/identityIcon.js

    const { iconsrc } = this.state;
-    const size = inline ? '32px' : '56px';
-    const classes = `${styles.icon} ${center ? styles.center : styles.left} ${padded ? styles.padded : ''} ${inline ? styles.inline : ''} ${className}`;
+    const classes = `${styles.icon} ${center ? styles.center : styles.left} ${padded ? styles.padded : ''} ${inline ? styles.inline : ''} ${button ? styles.button : ''} ${className}`;


Use an array? :)

Like that, should have done it from the start - but kept adding more and more cruft. Will fix.

derhuerst · 2016-09-15T16:30:09Z

js/src/ui/Modal/modal.css

@@ -23,3 +23,7 @@
  margin: 0;
  text-transform: uppercase;
 }
+
+.waiting {
+  margin: 0 -1em 0 -1em;


That is not very modular (;

Not 100% with you here.

derhuerst · 2016-09-15T16:33:13Z

Sry for the requested & approved changes spam, i'm still figuring out how to use this. 😄

jacogr added 21 commits September 13, 2016 23:32

initial import before tweaking starts

21c135f

GPL header

fc304c0

es6-ify library & flatten

6faff8f

fix tests with new structure, proper naming

1a6daa8

define functions explicitly withing closure

cab8a6b

call is not needed within closure

186baff

Merge branch 'js-3rdparty-shapeshift' into jg-fund-account

ea5e54a

* js-3rdparty-shapeshift: call is not needed within closure define functions explicitly withing closure fix tests with new structure, proper naming es6-ify library & flatten GPL header initial import before tweaking starts

allow className in Select

b2771fe

add missing provider logos

cc87e5e

actually export the newError action

24fff44

add support for buttons

be99b09

coin retrieval available

bfebf03

allow subscriptions to statusses

39f736a

simplify subscription state handling

3cca307

fatal error indicator

0b71fc8

render modal titles correctly in the case where there are no steps

b09c7c9

fix children propType

03b3a36

allow intermediate progress bar to be displayed on top on modals

937f3d2

UI looks reasonably presentable, testing with network next

3e5c8df

remove testing price info as well

62c4fc7

3 decimal places, display is unwieldly

581efd8

jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M-UI labels Sep 15, 2016

jacogr added 6 commits September 15, 2016 10:33

remove debug for no coins available

7698927

update coin message slightly

81c8eed

treat coin changes properly

885d321

busy shifting flag

ab7faf2

pass correct result through to callback

8b6b07c

fix PropTypes as received

902c8ac

jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Sep 15, 2016

jacogr added 2 commits September 15, 2016 12:16

slightly more presentable UI...

38e2e42

removed etherscan logo, doesn't belong in this branch

26d7b27

jacogr mentioned this pull request Sep 15, 2016

Misc small UI fixes (recently broken) #2101

Merged

derhuerst reviewed Sep 15, 2016

View reviewed changes

jacogr added 3 commits September 15, 2016 17:57

fix typo

5adc791

use shapeshift ison & name

a3bea1f

rename fundAccount to shapeshift

9703f4b

rphmeier reviewed Sep 15, 2016

View reviewed changes

fix wording

369334c

derhuerst suggested changes Sep 15, 2016

View reviewed changes

derhuerst approved these changes Sep 15, 2016

View reviewed changes

jacogr added 4 commits September 15, 2016 18:53

.map -> .forEach

f1fac42

array for classlist

268f4ef

check response.ok, throw Error

2981c87

Merge branch 'js' into jg-fund-account

001d888

derhuerst added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 15, 2016

ngotchac merged commit 7921fa8 into js Sep 15, 2016

ngotchac deleted the jg-fund-account branch September 15, 2016 18:13

derhuerst mentioned this pull request Sep 26, 2016

method signature lookups, parameter decoding & management #2313

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fund account via ShapeShift #2099

fund account via ShapeShift #2099

jacogr commented Sep 15, 2016 •

edited

Loading

parity-cla-bot commented Sep 15, 2016

derhuerst left a comment

derhuerst Sep 15, 2016

jacogr Sep 15, 2016

jacogr commented Sep 15, 2016 •

edited

Loading

rphmeier Sep 15, 2016

jacogr Sep 15, 2016

derhuerst Sep 15, 2016 •

edited

Loading

jacogr Sep 15, 2016

derhuerst Sep 15, 2016

jacogr Sep 15, 2016

derhuerst Sep 15, 2016

jacogr Sep 15, 2016

derhuerst Sep 15, 2016

jacogr Sep 15, 2016

derhuerst Sep 15, 2016

jacogr Sep 15, 2016

derhuerst Sep 15, 2016

jacogr Sep 15, 2016

derhuerst Sep 15, 2016

jacogr Sep 15, 2016

derhuerst Sep 15, 2016

derhuerst Sep 15, 2016

jacogr Sep 15, 2016

derhuerst Sep 15, 2016

jacogr Sep 15, 2016

derhuerst Sep 15, 2016

derhuerst Sep 15, 2016

jacogr Sep 15, 2016

derhuerst Sep 15, 2016

jacogr Sep 15, 2016

derhuerst commented Sep 15, 2016

fund account via ShapeShift #2099

fund account via ShapeShift #2099

Conversation

jacogr commented Sep 15, 2016 • edited Loading

parity-cla-bot commented Sep 15, 2016

derhuerst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacogr commented Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derhuerst Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derhuerst commented Sep 15, 2016

jacogr commented Sep 15, 2016 •

edited

Loading

jacogr commented Sep 15, 2016 •

edited

Loading

derhuerst Sep 15, 2016 •

edited

Loading