satellite/payments: token deposit #3283

rikysya · 2019-10-15T17:16:38Z

What: add functionality to deposit tokens

Why:

Please describe the tests:

Test 1:
Test 2:

Please describe the performance impact:

Code Review Checklist (to be filled out by reviewer)

Does the PR describe what changes are being made?
Does the PR describe why the changes are being made?
Does the code follow our style guide?
Does the code follow our testing guide?
Is the PR appropriately sized? (If it could be broken into smaller PRs it should be)
Does the new code have enough tests? (every PR should have tests or justification otherwise. Bug-fix PRs especially)
Does the new code have enough documentation that answers "how do I use it?" and "what does it do?"? (both source documentation and higher level, diagrams?)
Does any documentation need updating?
Do the database access patterns make sense?

…ents_token_deposit

ifraixedes

Some changes to be applied right away through Github code snippet suggestions.
The rest of the comments are questions.

ifraixedes · 2019-10-16T16:43:38Z

satellite/payments/coinpayments/utils.go

+
+	key := u.Query().Get("key")
+	if key == "" {
+		return "", errs.New("no key value found")


I think that's good to have a class or a sentinel error value for this matter.

ifraixedes · 2019-10-16T16:45:39Z

satellite/payments/coinpayments/utils.go

+)
+
+// GetTransacationKeyFromURL parses provided raw url string
+// and extracts authorization key from it.


I would add to this documentation that an error is returned if the URL is from an invalid format or it doesn't have an authorization key.

In case that you create the commented error class or sentinel error value, mention that in case that such error is returned when there is no authorization key.

satellite/payments/coinpayments/utils.go

satellite/payments/stripecoinpayments/tokens.go

satellite/payments/stripecoinpayments/transactions.go

ifraixedes · 2019-10-16T17:33:08Z

satellite/satellitedb/coinpaymentstxs.go

+func (db *coinpaymentsTransactions) Insert(ctx context.Context, tx stripecoinpayments.Transaction) (*stripecoinpayments.Transaction, error) {
+	amount, err := tx.Amount.GobEncode()
+	if err != nil {
+		return nil, err


Shouldn't we have to wrap all the errors returned by the methods of this type?

Don't we have a wrapper for all dbx errors? If that is the policy we follow that I will add another error

Checking dbx autogenerated code could be because there is an option to set a wrap error function. I don't know if it's set, I suppose that it's

Anyway, this one and some others of this function aren't errors returned by DBX so they should be wrapped.

oh, yeah you are right

ifraixedes · 2019-10-16T17:35:44Z

satellite/payments/tokens.go

+
+// TransactionStatus defines allowed statuses
+// for deposit transactions.
+type TransactionStatus string


I'm wondering, for consistency reasons, if this type shouldn't be an int like

storj/satellite/payments/coinpayments/transactions.go

Line 41 in acd51e4

type Status int

coinpayments is an implementation detail, it could possibly be different provider for token payment, with different status type. My idea is that it might be easier for caller to work with string value of statuses, but yeah it might be no difference actually.

Co-Authored-By: Ivan Fraixedes <ifcdev@gmail.com>

satellite/satellitedb/coinpaymentstxs.go

Co-Authored-By: Ivan Fraixedes <ifcdev@gmail.com>

…ents_token_deposit

ifraixedes

Just an improvement in the doc function comment but it's good to go.

satellite/payments/coinpayments/utils.go

Co-Authored-By: Ivan Fraixedes <ifcdev@gmail.com>

…ents_token_deposit

egonelbre · 2019-10-17T13:43:42Z

satellite/payments/stripecoinpayments/tokens.go

+	"storj.io/storj/satellite/payments/coinpayments"
+)
+
+// hack to ensure that storjTokens implements payments.StorjTokens.


This is not really an hack, since it's so widely used :D.

egonelbre · 2019-10-17T13:44:18Z

satellite/payments/stripecoinpayments/transactions_test.go

+
+		transactions := db.CoinpaymentsTransactions()
+
+		t.Run("test insert", func(t *testing.T) {


No need to rewrite "test", since it's running in a test, it's obviously a test :)

makes sense)

rikysya added 2 commits October 15, 2019 19:53

satellite/payments: storj token deposit

d9fe18c

add migration

ffd5fc0

rikysya added Request Code Review Code review requested Sprint Release Goal Sprint Release Goal labels Oct 15, 2019

rikysya requested a review from a team October 15, 2019 17:16

rikysya self-assigned this Oct 15, 2019

cla-bot bot added the cla-signed label Oct 15, 2019

ghost requested review from bryanchriswhite and thepaul and removed request for a team October 15, 2019 17:16

rikysya requested review from egonelbre and crawter October 15, 2019 17:16

crawter requested a review from ifraixedes October 16, 2019 08:11

Merge remote-tracking branch 'upstream/master' into yv/satellite/paym…

ea0df06

…ents_token_deposit

ifraixedes added the Code Review In-Progress Code review in-progress label Oct 16, 2019

rikysya added 3 commits October 16, 2019 15:43

fix build

8fd0bb9

update migration

f89f2dd

update precision of float to string conversion in coinpayments client

acd51e4

ifraixedes requested changes Oct 16, 2019

View reviewed changes

ifraixedes removed the Code Review In-Progress Code review in-progress label Oct 16, 2019

rikysya and others added 4 commits October 17, 2019 14:26

Update satellite/payments/stripecoinpayments/transactions.go

a5dd13f

Co-Authored-By: Ivan Fraixedes <ifcdev@gmail.com>

Update satellite/payments/stripecoinpayments/tokens.go

436150e

Co-Authored-By: Ivan Fraixedes <ifcdev@gmail.com>

Update satellite/payments/coinpayments/utils.go

205c753

Co-Authored-By: Ivan Fraixedes <ifcdev@gmail.com>

Update satellite/payments/stripecoinpayments/tokens.go

02d7ccc

Co-Authored-By: Ivan Fraixedes <ifcdev@gmail.com>

ifraixedes reviewed Oct 17, 2019

View reviewed changes

satellite/satellitedb/coinpaymentstxs.go Show resolved Hide resolved

rikysya and others added 4 commits October 17, 2019 14:53

Update satellite/satellitedb/coinpaymentstxs.go

30c0aeb

Co-Authored-By: Ivan Fraixedes <ifcdev@gmail.com>

fix comments

7f381e1

Merge remote-tracking branch 'upstream/master' into yv/satellite/paym…

cf16d98

…ents_token_deposit

add error to track missing auth key

0e88b96

rikysya requested a review from ifraixedes October 17, 2019 12:17

rikysya added 2 commits October 17, 2019 15:20

update comment

b7ca985

fix linter

1b67648

ifraixedes previously approved these changes Oct 17, 2019

View reviewed changes

satellite/payments/coinpayments/utils.go Outdated Show resolved Hide resolved

Update satellite/payments/coinpayments/utils.go

7a7aab6

Co-Authored-By: Ivan Fraixedes <ifcdev@gmail.com>

rikysya dismissed stale reviews from ifraixedes via 7a7aab6 October 17, 2019 13:08

Merge remote-tracking branch 'upstream/master' into yv/satellite/paym…

60b03c1

…ents_token_deposit

rikysya requested a review from ifraixedes October 17, 2019 13:10

ifraixedes previously approved these changes Oct 17, 2019

View reviewed changes

add hack to ensure we implement payments.StorjTokens

8393706

rikysya dismissed ifraixedes’s stale review via 8393706 October 17, 2019 13:22

rikysya requested a review from ifraixedes October 17, 2019 13:23

egonelbre previously approved these changes Oct 17, 2019

View reviewed changes

fix comments

35233d6

rikysya dismissed egonelbre’s stale review via 35233d6 October 17, 2019 13:56

rikysya requested a review from egonelbre October 17, 2019 13:56

ifraixedes approved these changes Oct 17, 2019

View reviewed changes

egonelbre approved these changes Oct 17, 2019

View reviewed changes

rikysya merged commit 24e72f3 into storj:master Oct 17, 2019

rikysya deleted the yv/satellite/payments_token_deposit branch October 17, 2019 14:04

crawter approved these changes Oct 17, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

satellite/payments: token deposit #3283

satellite/payments: token deposit #3283

rikysya commented Oct 15, 2019

ifraixedes left a comment

ifraixedes Oct 16, 2019

ifraixedes Oct 16, 2019

ifraixedes Oct 16, 2019

rikysya Oct 17, 2019

ifraixedes Oct 17, 2019

rikysya Oct 17, 2019

ifraixedes Oct 16, 2019

rikysya Oct 17, 2019

ifraixedes left a comment

egonelbre Oct 17, 2019

egonelbre Oct 17, 2019

rikysya Oct 17, 2019


		transactions := db.CoinpaymentsTransactions()

		t.Run("test insert", func(t *testing.T) {

satellite/payments: token deposit #3283

satellite/payments: token deposit #3283

Conversation

rikysya commented Oct 15, 2019

Code Review Checklist (to be filled out by reviewer)

ifraixedes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifraixedes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment