-
Notifications
You must be signed in to change notification settings - Fork 39
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
Document beneficiary information for transactions #209
Conversation
2ff4269
to
8081d70
Compare
8081d70
to
3a02d12
Compare
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.
just small comments, nothing that major yet.
_transactions.md
Outdated
"amount": "3000", | ||
"currency": "USD" | ||
}, | ||
"destination": "[email protected]" |
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.
should we be using here an @uphold.com
email?
also - what if someone signs up with that email?
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 think @uphold.com
would be misleading. The goal here is to make it evident that this is an invite transaction, hence the generic and self-descriptive email address. That said, we could make it even more generic to drive that point home. I'll change to [email protected]
.
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.
"beneficiary": { | ||
"relationship": "child", | ||
"name": "Harper Wilson", | ||
"address": { |
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.
Should we have a US address? I'm cool with Braga 😎 But is it common to have API docs with Braga?
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.
It's a good practice to make the examples varied, to showcase the diversity of supported content and formats. If we repeat the same structures and content, we risk making our API seem more rigid than it really is, or creating uncertainty regarding how to represent content that deviates from the examples.
_transactions.md
Outdated
This is required if **both** the following conditions are met: | ||
|
||
- The transaction is either a withdrawal to an external account (crypto networks, SEPA bank accounts, etc.) or a transfer to another user; and | ||
- The transaction amount is over _$3000 USD_ (or over $1000 USD, if the origin user is from Arizona). |
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.
Better to reference Arizona, United States?
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.
Sure, will change.
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.
Done.
_transactions.md
Outdated
- The transaction amount is over _$3000 USD_ (or over $1000 USD, if the origin user is from Arizona). | ||
|
||
<aside class="notice"> | ||
The threshold value is calculated from the <a href="#normalized">normalized</a> amount of the transaction, |
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.
Should we mention that it's the amount normalized to USD?
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.
Good point, will change.
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.
Done.
_transactions.md
Outdated
The special value `myself` can also be used when applicable. | ||
|
||
Depending on the conditions of the transaction, additional fields may be required | ||
— namely, the beneficiary `name` and `address` (with subfields `city`, `country`, `line1`, `line2`, `state`, `zipCode`), |
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.
Should we state which fields are mandatory here?
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.
We don't want to duplicate the logic of the code into the docs, both to minimize the possibility of them becoming out of sync, and to avoid making the docs too complex (since the actual requirements depend on the interaction of various conditions). The goal here was to explain the behavior of the API, and let the API responses provide the specific conditions.
_transactions.md
Outdated
and personal transactions (indicated by the values `child`, `co_worker`, `friend`, `parent`, or `sibling`). | ||
The special value `myself` can also be used when applicable. | ||
|
||
Depending on the conditions of the transaction, additional fields may be required |
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.
Is this newline with a dash intentional?
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.
It is intentional. I've been making use of semantic line breaks in the docs to facilitate reading of the the source files and make diffs more concise and readable, though I haven't applied that consistently throughout the repo yet. I can remove this particular instance if it's jarring to you.
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.
Oh, that's genius. Great concept ❤️
Anyway, I think what threw me off was the dash (-
) itself. Maybe we can just remove it?
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.
Yeah, there is some discussion about whether/how to break on dashes — it's certainly not a clear-cut case. I'll go ahead and remove this break.
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.
Done.
3a02d12
to
34ed607
Compare
34ed607
to
98edf50
Compare
@afsampaio we also need a list of the possible |
e2da129
to
98edf50
Compare
db9409a
to
e6b11ae
Compare
Deprecated by #219. |
Add a new section under the "Transactions" heading, describing the beneficiary information requirements.