Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

[Claim - Approve] -> USDC #2154

Merged
merged 5 commits into from
Jan 14, 2022
Merged

[Claim - Approve] -> USDC #2154

merged 5 commits into from
Jan 14, 2022

Conversation

W3stside
Copy link
Contributor

Summary

Part of #2088 and #2087 and #2089 and closes #2151

image

Testing

  1. needs acct with USDC and preferably GNO
  2. approve USDC

@W3stside W3stside requested review from alfetopito, anxolin and a team January 14, 2022 17:27
@@ -79,3 +79,9 @@ export const GNO: Record<number, Token> = {
'Gnosis'
),
}

export const USDC_BY_CHAIN: Record<SupportedChainId, Token> = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

called USDC_BY_CHAIN because USDC is a native const used across the app in uni code.

@W3stside W3stside mentioned this pull request Jan 14, 2022
@github-actions
Copy link
Contributor

  • 🔭 GP Swap: Gnosis Protocol v2 Swap UI

@W3stside
Copy link
Contributor Author

@anxolin @nenadV91 please review after merge

Copy link
Contributor

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

USDC approval working nicely

Comment on lines +253 to +260
gnoApproveData={{
approveCallback: gnoApproveCallback,
approveState: gnoApproveState,
}}
usdcApproveData={{
approveCallback: usdcApproveCallback,
approveState: usdcApproveState,
}}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine for now, but I'd rather have the approval logic inside the claim component.
We don't need to have this at the index, or do we?

Also, I still think we can do a generic approval flow rather than one for usdc and one for gno always even when they are not used.

Refactor for another day I guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably easier, just didnt like "mapping" components having this logic

@alfetopito alfetopito merged commit bd559e6 into claim Jan 14, 2022
@alfetopito alfetopito deleted the claim-usdc-approve branch January 14, 2022 18:00
Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

Only some small detail, can we just change a bit the description so we say Approve investing vCOW using USDC for example?

image

@W3stside
Copy link
Contributor Author

Only some small detail, can we just change a bit the description so we say Approve investing vCOW using USDC for example?

image

done b7cea63

@elena-zh
Copy link

LGTM!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants