Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make export TransactionOptions happen #392

Closed

Conversation

wi-ski
Copy link
Contributor

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

I need this for a plugin

@wi-ski
Copy link
Contributor Author

wi-ski commented Mar 16, 2020

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

@pinheadmz
Copy link
Member

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

@wi-ski
Copy link
Contributor Author

wi-ski commented Mar 16, 2020

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
Copy link

nijynot commented Mar 16, 2020

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
Copy link
Contributor

tynes commented Mar 16, 2020

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
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
Copy link
Contributor Author

wi-ski commented Mar 17, 2020

Updated 👌

@codecov-io
Copy link

codecov-io commented Mar 17, 2020

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
Copy link
Contributor Author

wi-ski commented Mar 17, 2020

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

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

@pinheadmz
Copy link
Member

pinheadmz commented Mar 17, 2020

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 👍

Copy link
Member

@pinheadmz pinheadmz left a comment

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
Copy link
Member

@pinheadmz pinheadmz left a comment

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

Copy link
Contributor

@boymanjor boymanjor left a comment

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
Copy link
Contributor Author

wi-ski commented Mar 25, 2020

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

@@ -1693,5 +1693,5 @@ function enforce(value, msg) {
/*
* Expose
*/

HTTP.TransactionOptions = TransactionOptions;
Copy link
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