Skip to content

Make export TransactionOptions happen - #392

Closed
wi-ski wants to merge 1 commit into
handshake-org:masterfrom
wi-ski:export-transactionoptions
Closed

Make export TransactionOptions happen#392
wi-ski wants to merge 1 commit into
handshake-org:masterfrom
wi-ski:export-transactionoptions

Conversation

@wi-ski

@wi-ski wi-ski commented Mar 16, 2020

Copy link
Copy Markdown
Contributor

I need this for a plugin

@wi-ski

wi-ski commented Mar 16, 2020

Copy link
Copy Markdown
Contributor Author

@tynes -Is this awful? Can i have this?

@pinheadmz

Copy link
Copy Markdown
Member

Can you expand on your use case a bit? The options are internal to the http module.

@wi-ski

wi-ski commented Mar 16, 2020

Copy link
Copy Markdown
Contributor Author

Ah - excuse me. I was in a headspace that made me feel like this would be an intended pattern.

Im building a plugin and would like to add a new route. Im copy-pasting a handler (in essence) and it would make use of this class - I had naively assumed it (meaning the class) would be exported from somewhere.

@nijynot

nijynot commented Mar 16, 2020

Copy link
Copy Markdown

Probably worth creating a separate issue on how to handle exporting a class' options it's not possible to use both module.exports and exports.* at the same time unless you're using ES6 imports.
Don't know if adding the option class to the HTTP class is desireable.

@tynes

tynes commented Mar 16, 2020

Copy link
Copy Markdown
Contributor

Don't know if adding the option class to the HTTP class is desireable.

I don't see a particular problem with it as a similar pattern is done elsewhere in the codebase. Its also not an object that is created millions of times.

@wi-ski Could you detail the new route you are working on? You also have a linting error

@pinheadmz

Copy link
Copy Markdown
Member

I agree with @tynes

Consider the style used here:

hsd/lib/primitives/mtx.js

Lines 1923 to 1932 in 98b33cc

/*
* Expose
*/
exports = MTX;
exports.MTX = MTX;
exports.Selector = CoinSelector;
exports.FundingError = FundingError;
module.exports = exports;

Also yeah please add the ; and please reword your commit message to match the style,
e.g. "wallet: expose transactionOptions in http"

@wi-ski
wi-ski force-pushed the export-transactionoptions branch 2 times, most recently from 7d179a2 to 8ba55b6 Compare March 17, 2020 01:11
@wi-ski

wi-ski commented Mar 17, 2020

Copy link
Copy Markdown
Contributor Author

Updated 👌

@codecov-io

codecov-io commented Mar 17, 2020

Copy link
Copy Markdown

Codecov Report

Merging #392 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #392      +/-   ##
==========================================
- Coverage   60.51%   60.51%   -0.01%     
==========================================
  Files         129      129              
  Lines       34750    34751       +1     
  Branches     5879     5879              
==========================================
  Hits        21029    21029              
- Misses      13721    13722       +1     
Impacted Files Coverage Δ
lib/wallet/http.js 65.04% <100.00%> (+0.03%) ⬆️
lib/covenants/rules.js 73.04% <0.00%> (-0.15%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5868dd6...fc1b47e. Read the comment docs.

@wi-ski

wi-ski commented Mar 17, 2020

Copy link
Copy Markdown
Contributor Author

@tynes - Im porting the changes from this PR: #233

Im setting up http handlers for stuff like /update-for-account and /reveal-for-account

Comment thread lib/wallet/http.js
@pinheadmz

pinheadmz commented Mar 17, 2020

Copy link
Copy Markdown
Member

Tested ok in REPL:

> const hsd = require('hsd')
undefined
> const Validator = require('bval')
undefined
> new hsd.wallet.HTTP.TransactionOptions(new Validator({rate: 123456}))
TransactionOptions {
  rate: 123456,
  maxFee: null,
  selection: null,
  smart: null,
  account: null,
  locktime: null,
  sort: null,
  subtractFee: null,
  subtractIndex: null,
  depth: null,
  paths: null,
  outputs: []
}
> new hsd.wallet.HTTP.TransactionOptions(new Validator({rate: 'help'}))
Thrown:
ValidationError: rate must be a int.
    at Validator.int (/Users/matthewzipkin/Desktop/work/hsd/node_modules/bval/lib/validator.js:171:13)
    at Validator.uint (/Users/matthewzipkin/Desktop/work/hsd/node_modules/bval/lib/validator.js:192:24)
    at Validator.u64 (/Users/matthewzipkin/Desktop/work/hsd/node_modules/bval/lib/validator.js:469:17)
    at TransactionOptions.fromValidator (/Users/matthewzipkin/Desktop/work/hsd/lib/wallet/http.js:1629:23)
    at new TransactionOptions (/Users/matthewzipkin/Desktop/work/hsd/lib/wallet/http.js:1616:19) {
  type: 'ValidationError',
  message: 'rate must be a int.'
}

Just fix that newline nit when you get a chance and I'll 👍

Comment thread lib/wallet/http.js

@pinheadmz pinheadmz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

great job @wi-ski thanks for hanging in there during review.

@pinheadmz
pinheadmz self-requested a review March 17, 2020 17:01

@pinheadmz pinheadmz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok just sqaush these into one commit actually

@boymanjor boymanjor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@wi-ski @pinheadmz @tynes I'd like us to prioritize getting #233 merged. Is this PR necessary once we get it in? I'm not sure we expose any other internal, options classes. However, any plugins that wants to create tx related endpoints could benefit from this.

@wi-ski
wi-ski force-pushed the export-transactionoptions branch from 7cbb45c to fc1b47e Compare March 25, 2020 00:15
@wi-ski

wi-ski commented Mar 25, 2020

Copy link
Copy Markdown
Contributor Author

@pinheadmz - Squashed 👐 Lemme know if there's anything else.

Comment thread lib/wallet/http.js
* Expose
*/

HTTP.TransactionOptions = TransactionOptions;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

dude, sorry to be super annoying about this, but the newline should stay in :-)
I think when you squashed you might've removed the commit that added the newline.

@nodech nodech added the has PR issue - issue has PR label Dec 9, 2021
@nodech nodech closed this in #670 Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

has PR issue - issue has PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants