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

feat: add isSponsored prop for Swap #1293

Merged
merged 6 commits into from
Sep 24, 2024
Merged

feat: add isSponsored prop for Swap #1293

merged 6 commits into from
Sep 24, 2024

Conversation

0xAlec
Copy link
Contributor

@0xAlec 0xAlec commented Sep 24, 2024

What changed? Why?

  • add isSponsored prop

Notes to reviewers

How has it been tested?

Copy link

vercel bot commented Sep 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
onchainkit-coverage ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 24, 2024 7:22pm
onchainkit-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 24, 2024 7:22pm
onchainkit-routes ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 24, 2024 7:22pm

@0xAlec 0xAlec changed the title Alec/paymaster swap feat: add isSponsored prop for Swap Sep 24, 2024
@@ -21,6 +21,7 @@ export function Swap({
onError,
onStatus,
onSuccess,
isSponsored,
Copy link
Contributor

Choose a reason for hiding this comment

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

could default to false here and not have to handle undefined

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 don't think we need special handling for undefined since it's falsey

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but I can add a default 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.

added in latest diff

Copy link
Contributor

Choose a reason for hiding this comment

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

I still like explicitly defining as false

@@ -267,6 +269,7 @@ export type SwapProviderReact = {
experimental: {
useAggregator: boolean; // Whether to use a DEX aggregator. (default: true)
};
isSponsored?: boolean; // Whether the swap is sponsored. (default: false)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: starting the comment description with // An optional...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -282,6 +285,7 @@ export type SwapReact = {
experimental?: {
useAggregator: boolean; // Whether to use a DEX aggregator. (default: true)
};
isSponsored?: boolean; // Whether the swap is sponsored. (default: false)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: starting the comment description with // An optional...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -48,6 +49,7 @@ export function Swap({
onError={onError}
onStatus={onStatus}
onSuccess={onSuccess}
isSponsored={isSponsored}
Copy link
Contributor

@cpcramer cpcramer Sep 24, 2024

Choose a reason for hiding this comment

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

nit: alphabetic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done - this was from when we called it sponsored

@@ -21,6 +21,7 @@ export function Swap({
onError,
onStatus,
onSuccess,
isSponsored = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: alphabetic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done - this was from when we called it sponsored

@abcrane123 abcrane123 merged commit 7b33028 into main Sep 24, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants