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

Add transaction_id field to SEP-12 requests #165

Merged
merged 4 commits into from
Sep 2, 2024
Merged

Conversation

CassioMG
Copy link
Contributor

Also improves typing by adding a separate CustomerBinaryInfoMap type only for the binary (Buffer) fields.

Closes: Add transaction id to SEP-12 requests

Also improves typing by adding a separate CustomerBinaryInfoMap type only for the binary (Buffer) fields
@CassioMG CassioMG changed the title Add 'transaction_id' field to SEP-12 requests Add transaction_id field to SEP-12 requests Aug 23, 2024
@@ -78,28 +83,38 @@ export class Sep12 {
* @param {AddCustomerParams} params - The parameters for adding a customer.
* @param {CustomerInfoMap} [params.sep9Info] - Customer information. What fields you should
* give is indicated by the anchor.
* @param {CustomerInfoMap} [params.sep9BinaryInfo] - Customer information that is in binary
* @param {CustomerBinaryInfoMap} [params.sep9BinaryInfo] - Customer information that is in binary
Copy link
Contributor

@Ifropc Ifropc Aug 26, 2024

Choose a reason for hiding this comment

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

How does passing Buffer vs passing a string will work?
My question, is how exactly both of this types will be included in the request?
Can we add a unit test that makes sure that the body layout is correct (I believe type of binary fields should NOT be text/plain) when passing either string or buffer
And in the future add an integration test with Anchor Platform to pass any of binary fields?

(I see we currently have tests with Polaris testanchor but it might just ignore this fields, so I don't think it's a very trustworthy test)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ifropc thanks for the review!
This is how it currently works: any string field should be passed through the sep9Info object and any binary field (e.g. KYC images) should be passed through the sep9BinaryInfo object.
If you pass any field on the sep9BinaryInfo object then the request Content-Type header will be set to multipart/form-data.

How does that sound?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was a bit confused by

export type CustomerBinaryInfoMap = {
  [key: string]: Buffer | string;
};

My guess is that it will have an appropriate body layout depending on the type of the value in the map?
In any case, adding a unit test would be a great addition.

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 was a bit confused by

export type CustomerBinaryInfoMap = {
  [key: string]: Buffer | string;
};

Fixed this is: 6f38db4

Now the types look like this:

export type CustomerInfoMap = {
  [key: string]: string;
};

export type CustomerBinaryInfoMap = {
  [key: string]: Buffer;
};

My guess is that it will have an appropriate body layout depending on the type of the value in the map?
In any case, adding a unit test would be a great addition.

Created a task to tackle this unit test: https://github.com/orgs/stellar/projects/58/views/1?pane=issue&itemId=78332363

Copy link
Contributor

@Ifropc Ifropc left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for clarifying comment

@CassioMG CassioMG merged commit 44311dc into main Sep 2, 2024
8 checks passed
@CassioMG CassioMG deleted the cg-sep12-tx-id branch September 2, 2024 17:27
@CassioMG CassioMG mentioned this pull request Nov 8, 2024
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