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

chore: payment flow restructure #37

Merged
merged 5 commits into from
Jan 8, 2024

Conversation

ionutanin
Copy link
Contributor

  • update payment flow
  • move payment methods into separate files in preparation to apply unit tests
  • remove unused files and code

@ionutanin ionutanin added the enhancement New feature or request label Dec 6, 2023
@ionutanin ionutanin self-assigned this Dec 6, 2023
@ionutanin ionutanin changed the title Payment flow restructure chore: payment flow restructure Dec 6, 2023
Copy link
Member

@raducristianpopa raducristianpopa left a comment

Choose a reason for hiding this comment

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

  1. We do not have types at all. What's your opinion about fetching the OpenAPI schemas from the Open Payments repository and generating the types from them?
  2. We should probably move from Axios to the Fetch API or find a way to use the OP SDK
  3. I think that whole flow is too granular (split into too many files)

Comment on lines 16 to 20
if (tabId === tab.id && changeInfo.url?.includes('interact_ref')) {
const interactRef = changeInfo.url.split('interact_ref=')[1]
tabs.update(currentTabId, { active: true })
tabs.remove(tab.id)
resolve(interactRef)
Copy link
Member

Choose a reason for hiding this comment

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

Might be better to use the URL object so we can also validate the URL. This way we can avoid using split or includes and take advantage of searchParams.get from the URL object to retrieve the interaction reference.

Comment on lines 5 to 11
export const createQuote = async (
receiver: string,
walletAddress: string,
sendingUrl: string,
token: string,
instance: AxiosInstance,
) => {
Copy link
Member

Choose a reason for hiding this comment

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

We should use an object as a single parameter for functions that have more than 2 parameters.

receiver,
walletAddress,
debitAmount: {
value: '1000000', // 0.001 USD
Copy link
Member

Choose a reason for hiding this comment

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

The amount value will need to be calculated based on the hourly rate, which the user has control on. Can the amount value be passed as a parameter. Ideally we should now the exact value before quoting (storing it somewhere) so we won't have to recalculate it every time.

Comment on lines 18 to 19
assetCode: 'USD',
assetScale: 9,
Copy link
Member

Choose a reason for hiding this comment

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

Asset information needs to be fetched from the sender's wallet address. Since we have a setup/on-boarding step we probably can save these values somewhere.

Comment on lines 5 to 10
export const getContinuationRequest = async (
url: string,
interactRef: string,
token: string,
instance: AxiosInstance,
) => {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto above:

We should use an object as a single parameter for functions that have more than 2 parameters.

Comment on lines 3 to 9
export const getOutgoingPaymentGrant = async (
client: string,
identifier: string,
wallet: Record<string, any>,
amount: string | number,
instance: AxiosInstance,
) => {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto above

access: [
{
type: 'outgoing-payment',
actions: ['list', 'list-all', 'read', 'read-all', 'create'],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
actions: ['list', 'list-all', 'read', 'read-all', 'create'],
actions: ['list', 'read', 'create'],

Comment on lines 22 to 24
value: String(Number(amount) * 10 ** 9), // '1000000000',
assetScale: 9,
assetCode: 'USD',
Copy link
Member

Choose a reason for hiding this comment

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

Ditto above (the asset information).

Comment on lines 3 to 8
export const getQuoteGrant = async (
client: string,
identifier: string,
wallet: Record<string, any>,
instance: AxiosInstance,
): Promise<string> => {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto above.


import { getHeaders } from '@/background/grant/getHeaders'

export const rotateToken = async (url: string, token: string, instance: AxiosInstance) => {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto above.

@github-actions github-actions bot added the area: background Improvements or additions to extension background script label Dec 12, 2023
@raducristianpopa
Copy link
Member

raducristianpopa commented Dec 12, 2023

Extension builds preview

Name Link
Latest commit 7aa860e
Latest job logs Run #7447027580
BadgeDownload
BadgeDownload
BadgeDownload
BadgeDownload


type TCreateQuote = (_params: {
receiver: string
walletAddress: any
Copy link
Member

Choose a reason for hiding this comment

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

What actually gets passed in here? Let's avoid using any and create some small types until we decide how we should use the OpenAPI types.

token: string
amount: string
instance: AxiosInstance
}) => Promise<any>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}) => Promise<any>
}) => Promise<string>


type TGetContinuationRequest = (_params: {
url: string
interactRef: any
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
interactRef: any
interactRef: string

interactRef: any
token: string
instance: AxiosInstance
}) => Promise<any>
Copy link
Member

Choose a reason for hiding this comment

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

Let's type the response.

type TGetIncomingPaymentGrant = (_params: {
client: string
identifier: string
wallet: Record<string, any>
Copy link
Member

Choose a reason for hiding this comment

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

Ditto above.

walletAddress: string
token: string
instance: AxiosInstance
}) => Promise<any>
Copy link
Member

Choose a reason for hiding this comment

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

Ditto above.

type TGetOutgoingPaymentGrant = (_params: {
client: string
identifier: string
wallet: Record<string, any>
Copy link
Member

Choose a reason for hiding this comment

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

Ditto above.

wallet: Record<string, any>
amount: string | number
instance: AxiosInstance
}) => Promise<any>
Copy link
Member

Choose a reason for hiding this comment

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

Ditto above.

Comment on lines +6 to +8
wallet: Record<string, any>
instance: AxiosInstance
}) => Promise<any>
Copy link
Member

Choose a reason for hiding this comment

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

Ditto above.

url: string
token: string
instance: AxiosInstance
}) => Promise<any>
Copy link
Member

Choose a reason for hiding this comment

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

Ditto above.

@raducristianpopa raducristianpopa merged commit 4aa460a into main Jan 8, 2024
9 checks passed
@raducristianpopa raducristianpopa deleted the chore/payment-flow-restructure branch January 8, 2024 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: background Improvements or additions to extension background script enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code cleaning and move all api methods into its own file (like in rafiki)
2 participants