Add fee selection to Open Channel form #182

jamaljsr · 2019-03-09T01:58:40Z

Closes #181

Description

Adds a new field to the Open Channel form giving the option to choose the transaction speed. This will set the sats/byte to a predetermined value based on the current BTC mainnet mempool.

Note: Fee estimates are only provided for BTC so this field is hidden when connected to an LTC node

Steps to Test

Click on Open channel from the top right nav menu
Click on the Show advanced fields button in the modal
Choose a fee level (auto/slow/normal/fast)
Confirm the sats/byte changes in the help message below the toggle
Click the Open Channel button to send the transaction
View the transaction in a block explorer to verify that your fee was used. You can open two channels to to confirm that choosing the Fast option is cheaper than Auto

Screenshots

jamaljsr · 2019-03-10T00:44:42Z

I added the ability to manually specify a custom fee (sats/byte) in the fee selection field. It also displays a warning if the specified fee is larger than the fastest recommended fee.

jamaljsr · 2019-03-10T01:03:48Z

I just committed an alternative UI using a slider instead of the toggle/input combo. I think this may be a better solution as it reduces the chances of someone fat-fingering a really high fee unnecessarily. It also greatly simplifies the error detection code.

If you prefer the previous UI, then I can easily revert the last commit. I just wanted to throw this out there as I felt it is a better approach.

wbobeirne · 2019-03-17T18:38:35Z

Love the new UI. Will be taking a look at this a little later, but definitely prefer this direction!

wbobeirne

Just worried about the chainsend comment, if that's all good then I'm 100% 👍 on this one. Great work!

wbobeirne · 2019-03-18T19:42:04Z

src/app/components/OpenChannelModal/index.tsx

        capacity,
        isPrivate,
+        // only send non-zero fee
+        fee: fee ? fee.toString() : undefined,


Does handleFeeSubmit in ChainSend need the same consideration here? Looks like it could send 0 right now.

wbobeirne · 2019-03-18T19:42:45Z

src/app/components/FeeSelectField/index.tsx

+              switch (v) {
+                case onChainFees.fastestFee: return 'fastest';
+                case onChainFees.halfHourFee: return 'normal';
+                case onChainFees.hourFee: return 'slow';


Would be nice if these kept the number as well, e.g. 16 (normal)

jamaljsr · 2019-03-18T21:41:53Z

Great feedback. Thanks for the review. I’ll get these couple items fixed up as soon as I can.

otech47 · 2019-03-20T11:34:06Z

src/app/components/FeeSelectField/index.tsx

+    this.props.getOnChainFeeEstimates();
+  }
+
+  componentWillReceiveProps(nextProps: Props) {	


FYI componentWillReceiveProps will be deprecated in React 17 and generally a good upgrade path for this kind of function is to use componentDidUpdate instead and compare against prevProps instead of nextProps

https://reactjs.org/docs/react-component.html#updating

More of a long term consideration but noticed it because I write a ton of React and have gotten used to it by now

Oh React, always deprecating lifecycles... Thanks for the heads up! We can probably let this one slip since we're using this lifecycle in quite a few places, and can just do one big refactor when we come around to v17.

otech47 · 2019-03-20T11:47:12Z

src/app/components/FeeSelectField/index.tsx

+    const { onChainFees } = this.props;	
+    if (nextProps.onChainFees !== onChainFees && nextProps.onChainFees !== null) {	
+      const { fastestFee } = nextProps.onChainFees;
+      this.setState({ fee: fastestFee });	


Also just took a look at the parent component and am wondering whether you wouldn’t need local state in this component and you can just pass the fee as a prop from the ChainSend component since it seems like you’re managing changes to both this local state and the parents local state?

All good if you have your reasoning that I don’t see, but in any case these are just architecture suggestions and not functionally critical.

Love the slider UI btw 👍👍👍

I like this idea, making it a controlled component rather than an uncontrolled one. Better matches the paradigm of most input components we have.

wbobeirne

@jamaljsr I've got some free time today so I'm going to tackle the PR feedback here. I'll push the changes up and wait for you to 👍 before merging in.

wbobeirne · 2019-03-20T17:28:33Z

src/app/components/FeeSelectField/index.tsx

+    const { onChainFees } = this.props;	
+    if (nextProps.onChainFees !== onChainFees && nextProps.onChainFees !== null) {	
+      const { fastestFee } = nextProps.onChainFees;
+      this.setState({ fee: fastestFee });	


I like this idea, making it a controlled component rather than an uncontrolled one. Better matches the paradigm of most input components we have.

wbobeirne · 2019-03-20T17:29:08Z

src/app/components/FeeSelectField/index.tsx

+    this.props.getOnChainFeeEstimates();
+  }
+
+  componentWillReceiveProps(nextProps: Props) {	


Oh React, always deprecating lifecycles... Thanks for the heads up! We can probably let this one slip since we're using this lifecycle in quite a few places, and can just do one big refactor when we come around to v17.

jamaljsr · 2019-03-20T19:14:44Z

hey @wbobeirne I am traveling and wont be able to make any changes to this branch until this weekend. Feel free to make any additional updates you think are improvements.

Great feedback @otech47, thank you very much.

* Make FeeSelectField a controlled component * Show number when hovered over labeled fee estimates * Fix ChainSend automatic fee calculation

…el-fee

wbobeirne · 2019-03-20T19:29:19Z

No worries, hope it's somewhere sunny. I just pushed changes to address the feedback, and merged develop back in to fix the snyk CI. If you don't get a chance to review, I'll probably merge as is since I tested it pretty thoroughly and it all LGTM.

jamaljsr · 2019-03-22T12:10:18Z

@wbobeirne Thanks for cleaning this up and getting it merged 👍

jamaljsr added 6 commits March 8, 2019 18:35

Extract the fee toggle into a component

db4011b

Add fee selection to Open Channel form

288da18

Hide fee selection field for LTC

68fb934

Fix order of import statements

bb78959

Add ability to manually enter a custom fee amount

23cd659

Fix modal re-centering issue in popup

297aa1b

Switch field to use a slider instead of a toggle

21c4787

wbobeirne reviewed Mar 18, 2019

View reviewed changes

otech47 reviewed Mar 20, 2019

View reviewed changes

wbobeirne reviewed Mar 20, 2019

View reviewed changes

wbobeirne added 2 commits March 20, 2019 15:25

Changes from PR feedback:

61058c8

* Make FeeSelectField a controlled component * Show number when hovered over labeled fee estimates * Fix ChainSend automatic fee calculation

Merge remote-tracking branch 'wbobeirne/develop' into feat/open-chann…

5787c29

…el-fee

wbobeirne approved these changes Mar 21, 2019

View reviewed changes

wbobeirne merged commit a6d756a into joule-labs:develop Mar 21, 2019

jamaljsr deleted the feat/open-channel-fee branch March 22, 2019 12:08

Add fee selection to Open Channel form #182

Add fee selection to Open Channel form #182

Uh oh!

Conversation

jamaljsr commented Mar 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Steps to Test

Screenshots

Uh oh!

jamaljsr commented Mar 10, 2019

Uh oh!

jamaljsr commented Mar 10, 2019

Uh oh!

wbobeirne commented Mar 17, 2019

Uh oh!

wbobeirne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamaljsr commented Mar 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbobeirne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamaljsr commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wbobeirne commented Mar 20, 2019

Uh oh!

jamaljsr commented Mar 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jamaljsr commented Mar 9, 2019 •

edited

Loading

jamaljsr commented Mar 20, 2019 •

edited

Loading