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

Feature/sep 6 deposit #163

Merged
merged 10 commits into from
May 24, 2021
Merged

Feature/sep 6 deposit #163

merged 10 commits into from
May 24, 2021

Conversation

piyalbasu
Copy link
Contributor

@piyalbasu piyalbasu commented May 20, 2021

No description provided.

@piyalbasu
Copy link
Contributor Author

I will not be merging this until we develop a release strategy for v1.2.0

description: `Start SEP-6 withdrawal of ${balance.assetCode}?`,
callback: () => handleSep24Withdraw(balance),
};
break;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this PR covers SEP-6 deposit of a trusted asset. A follow up PR will cover untrusted assets using claimable balances

if (sep6DepositAsset.status === ActionStatus.NEEDS_INPUT) {
setFormData({
depositType: {
type: sep6DepositAsset.data.depositTypes.type.choices[0],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

default formData to the first choice as some assets (like SRT) will only have one

@stellar-jenkins
Copy link

@stellar-jenkins
Copy link

AnyObject,
} from "types/types.d";

export const initiateSendAction = createAsyncThunk<
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is following the general pattern of SEP-24

@stellar-jenkins
Copy link

payload = {
...payload,
fields: { ...payload.fields, ...sep12Fields },
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add any required fields found in /customer endpoint

const { data } = sep6DepositSelector(getState());
const { kycServer, token } = data;

if (Object.keys(fields).length) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we have any customer info fields, submit them. Otherwise, just record the deposit type that the user selected

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we doing anything with submitted fields or the response?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're not - it looks like we just care about it not throwing an error. This request does respond with a generated id, but we don't appear to have a use for it, nor the fields that we submitted, as long as the response is a 200

@stellar-jenkins
Copy link

@@ -0,0 +1,31 @@
import { get } from "lodash";
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 didn't touch much code outside of SEP-6 flow, but I did combine the checkInfo methods for SEP-6 and SEP-24 as they are doing the same thing and expect the same payload

@@ -0,0 +1,54 @@
import { log } from "helpers/log";
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 also moved the SEP-12 methods into their own folder so SEP-6 and SEP-31 could both use them

@stellar-jenkins
Copy link


log.response({ title: "GET `/customer`", body: resultJson });

if (isNewCustomer && resultJson.status !== "NEEDS_INFO") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

one small change to this code: I added this isNewCustomer boolean as SEP-31 wants this method to throw error if it doesn't need more info. SEP-6, however, should continue working if no add'l info is needed

@piyalbasu piyalbasu requested a review from quietbits May 20, 2021 17:59
@stellar-jenkins
Copy link

Copy link
Contributor

@quietbits quietbits left a comment

Choose a reason for hiding this comment

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

Looks great, just left a few minor comments/questions. 🙌

const { sep6DepositAsset } = useRedux("sep6DepositAsset");
const { depositResponse } = sep6DepositAsset;

const [formData, setFormData] = useState<any>({
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could be more specific about the type here.

Comment on lines 119 to 120
<label>{input.description}</label>
<Select id={id} key={id} onChange={handleDepositTypeChange}>
Copy link
Contributor

Choose a reason for hiding this comment

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

<Select> has a label prop, can't we use it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, missed that! Yes, I should use the label prop

const { data } = sep6DepositSelector(getState());
const { kycServer, token } = data;

if (Object.keys(fields).length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we doing anything with submitted fields or the response?

} from "types/types.d";

export const initiateSendAction = createAsyncThunk<
{ fields: {}; status: ActionStatus },
Copy link
Contributor

Choose a reason for hiding this comment

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

We could use AnyObject type for fields.

@stellar-jenkins
Copy link

@piyalbasu piyalbasu changed the base branch from master to release/1.2.0 May 24, 2021 14:59
…sep-6-deposit

# Conflicts:
#	src/components/BalanceRow.tsx
@piyalbasu piyalbasu changed the title [DO NOT MERGE] Feature/sep 6 deposit Feature/sep 6 deposit May 24, 2021
…sep-6-deposit

# Conflicts:
#	src/components/BalanceRow.tsx
@stellar-jenkins
Copy link

1 similar comment
@stellar-jenkins
Copy link

@piyalbasu piyalbasu merged commit 4cfc56b into release/1.2.0 May 24, 2021
@piyalbasu piyalbasu deleted the feature/sep-6-deposit branch May 24, 2021 15:28
marcelosalloum pushed a commit that referenced this pull request Jul 19, 2021
* add Sep-6 deposit flow

* fix typo

* adding bad input handling for deposit choice

* reorder import

* rm unneeded eslint disable

* memoize depositTypes choices

* PR comments
marcelosalloum pushed a commit that referenced this pull request Jul 19, 2021
* add Sep-6 deposit flow

* fix typo

* adding bad input handling for deposit choice

* reorder import

* rm unneeded eslint disable

* memoize depositTypes choices

* PR comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants