Skip to content
This repository has been archived by the owner on Oct 30, 2018. It is now read-only.

[WIP] Coin payments setup #88

Merged
merged 39 commits into from Nov 9, 2017

Conversation

dylanlott
Copy link
Contributor

TO DO:

  • need to test credit creation logic
  • need unit tests for credit creation

@coveralls
Copy link

Coverage Status

Coverage decreased (-16.1%) to 32.563% when pulling e0cb847 on dylanlott:coin-payments into 43831f2 on Storj:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-16.1%) to 32.563% when pulling b638bf9 on dylanlott:coin-payments into 43831f2 on Storj:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-16.1%) to 32.563% when pulling dd5d534 on dylanlott:coin-payments into 43831f2 on Storj:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-16.1%) to 32.563% when pulling f18b801 on dylanlott:coin-payments into 43831f2 on Storj:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-16.1%) to 32.563% when pulling 7691da3 on dylanlott:coin-payments into 43831f2 on Storj:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-17.2%) to 31.513% when pulling 3030639 on dylanlott:coin-payments into 7d0c14b on Storj:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-17.2%) to 31.513% when pulling 86f2d48 on dylanlott:coin-payments into 7d0c14b on Storj:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-17.4%) to 31.347% when pulling 17aa771 on dylanlott:coin-payments into 710e784 on Storj:master.

Copy link
Contributor

@bryanchriswhite bryanchriswhite left a comment

Choose a reason for hiding this comment

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

Not trying to be a hard-ass... I did see you did some cleanup and conversion of console.log to log.<verb> already 👍

return log.error('no user found for ipn payment', req.body);
}

console.log('found payment processor: ', proc);
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is worth logging, let's use the logger, otherwise let's clean it up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


return credit.save();
} else {
credit.user = user;
Copy link
Contributor

Choose a reason for hiding this comment

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

When would this condition occur; a credit with the data.txn_id already exists? My concerns is exposing the ability for a user to reset a credit to {paid: false, paid_amount: 0, invoiced_amount: req.body.amount} by sending an authenticated request this endpoint with a txn_id of an arbitrary credit that exists.

Copy link
Contributor Author

@dylanlott dylanlott Nov 3, 2017

Choose a reason for hiding this comment

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

The IPN calls they make to our webhook are sort of unpredictable. Though you're right, they could spoof this. I'll think about a way to deal with this more today


// create a pending credit
if (req.body.status === 0) {
Credit.findOne({ 'data.txn_id': req.body.txn_id })
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we should add the user to the credit search criteria? I think this would prevent users from accessing other users credits via the api.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


if (req.body.status === 100) {
// search for pending credit and update it
Credit.findOne({ 'data.txn_id': req.body.txn_id })
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above; can we add user to search criteria

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Credit.findOne({ 'data.txn_id': req.body.txn_id })
.then(credit => {
if (!credit) {
const credit = new Credit({
Copy link
Contributor

Choose a reason for hiding this comment

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

When would this condition occur? Same concerns as above.

return log.error('no user found for ipn payment', req.body);
}

console.log('found payment processor: ', proc);
Copy link
Contributor

Choose a reason for hiding this comment

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

^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

paid_amount: 0,
type: 'automatic',
user: user,
payment_processor: 'coinpayments',
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use the new COINPAYMENTS constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

invoiced_amount: req.body.amount,
user: user,
paid: true,
payment_processor: 'coinpayments',
Copy link
Contributor

Choose a reason for hiding this comment

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

^ COINPAYMENTS constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

autoIpn: process.env.NODE_ENV === 'production' ? false : true
}

const client = new Coinpayments(options);
Copy link
Contributor

Choose a reason for hiding this comment

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

same feedback as in service-storage-models... in fact maybe you can reference service-storage-models' copy instead of duplicating?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered exporting Coinpayments client on the service-storage-models index. Still an option?

@@ -73,6 +73,7 @@
},
"dependencies": {
"async": "^1.5.2",
"coinpayments": "^1.1.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

^

@coveralls
Copy link

Coverage Status

Coverage decreased (-17.5%) to 31.299% when pulling 9f2abec on dylanlott:coin-payments into 710e784 on Storj:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-17.4%) to 31.421% when pulling 9f2abec on dylanlott:coin-payments into 710e784 on Storj:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-5.8%) to 42.494% when pulling 0dea05e on dylanlott:coin-payments into 3670c60 on Storj:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-5.8%) to 42.494% when pulling 948a972 on dylanlott:coin-payments into 3670c60 on Storj:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-6.4%) to 41.96% when pulling ddc3f80 on dylanlott:coin-payments into 3670c60 on Storj:master.

- Setup test harnesses for handleIPN route
- Added coinpayments router to routefactory
- Remove unnecessary logs
@coveralls
Copy link

Coverage Status

Coverage decreased (-5.9%) to 42.386% when pulling 0fd93d5 on dylanlott:coin-payments into 3670c60 on Storj:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.4%) to 45.965% when pulling b895fa0 on dylanlott:coin-payments into 3670c60 on Storj:master.

@bryanchriswhite
Copy link
Contributor

@dylanlott if tests are passing on your machine I'm down to merge and test in staging...

@bryanchriswhite bryanchriswhite merged commit 13f867b into storj-archived:master Nov 9, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-2.4%) to 45.965% when pulling 4a27f57 on dylanlott:coin-payments into 3670c60 on Storj:master.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants