-
Notifications
You must be signed in to change notification settings - Fork 8
[WIP] Coin payments setup #88
[WIP] Coin payments setup #88
Conversation
- added route handling for adding a wallet - tied wallet to user - setup constants for accepted wallet currencies - added error handling
- adds in coinpayment IPN middleware - sets up `/pp/coinpayments/` endpoint for IPN webhook
- adds removePaymentMethod to coinpayments adapter
There was a problem hiding this 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 👍
lib/server/routes/coinpayments.js
Outdated
return log.error('no user found for ipn payment', req.body); | ||
} | ||
|
||
console.log('found payment processor: ', proc); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
lib/server/routes/coinpayments.js
Outdated
|
||
return credit.save(); | ||
} else { | ||
credit.user = user; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
lib/server/routes/coinpayments.js
Outdated
|
||
// create a pending credit | ||
if (req.body.status === 0) { | ||
Credit.findOne({ 'data.txn_id': req.body.txn_id }) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
lib/server/routes/coinpayments.js
Outdated
|
||
if (req.body.status === 100) { | ||
// search for pending credit and update it | ||
Credit.findOne({ 'data.txn_id': req.body.txn_id }) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
lib/server/routes/coinpayments.js
Outdated
Credit.findOne({ 'data.txn_id': req.body.txn_id }) | ||
.then(credit => { | ||
if (!credit) { | ||
const credit = new Credit({ |
There was a problem hiding this comment.
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.
lib/server/routes/coinpayments.js
Outdated
return log.error('no user found for ipn payment', req.body); | ||
} | ||
|
||
console.log('found payment processor: ', proc); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
lib/server/routes/coinpayments.js
Outdated
paid_amount: 0, | ||
type: 'automatic', | ||
user: user, | ||
payment_processor: 'coinpayments', |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
lib/server/routes/coinpayments.js
Outdated
invoiced_amount: req.body.amount, | ||
user: user, | ||
paid: true, | ||
payment_processor: 'coinpayments', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ COINPAYMENTS
constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
lib/server/vendor/coinpayments.js
Outdated
autoIpn: process.env.NODE_ENV === 'production' ? false : true | ||
} | ||
|
||
const client = new Coinpayments(options); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^
- Setup test harnesses for handleIPN route - Added coinpayments router to routefactory - Remove unnecessary logs
@dylanlott if tests are passing on your machine I'm down to merge and test in staging... |
TO DO: