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

fix(orders-table): display allowance warning in Safe #5207

Merged
merged 7 commits into from
Dec 17, 2024
Merged

Conversation

shoom3301
Copy link
Collaborator

Summary

Fixes #4076

hasValidPendingPermit can be undefined

image

To Test

This issue was introduced in #3487, so please make sure this issue is not back.

See #4076

@shoom3301 shoom3301 requested review from a team December 16, 2024 10:06
@shoom3301 shoom3301 self-assigned this Dec 16, 2024
Copy link

vercel bot commented Dec 16, 2024

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

Name Status Preview Updated (UTC)
cosmos 🔄 Building (Inspect) Visit Preview Dec 17, 2024 4:18pm
cowfi 🔄 Building (Inspect) Visit Preview Dec 17, 2024 4:18pm
explorer-dev 🔄 Building (Inspect) Visit Preview Dec 17, 2024 4:18pm
sdk-tools 🔄 Building (Inspect) Visit Preview Dec 17, 2024 4:18pm
swap-dev 🔄 Building (Inspect) Visit Preview Dec 17, 2024 4:18pm
widget-configurator 🔄 Building (Inspect) Visit Preview Dec 17, 2024 4:18pm

@elena-zh
Copy link
Contributor

elena-zh commented Dec 16, 2024

Hey @shoom3301 , everything looks good besides one bug (#3480) that has become reproducible here again
issue is back

@shoom3301
Copy link
Collaborator Author

@elena-zh thank you! Fixed that.
@alfetopito could you check the fix please?
Mainly, I got rid of PendingPermitUpdater and everything related and replaced the logic with getOrderPermitAmount().
Now _hasEnoughBalanceAndAllowance takes an order permit value into account.

)

const withAllowanceWarning = hasEnoughAllowance === false && hasValidPendingPermit === false
const withAllowanceWarning = hasEnoughAllowance === false
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now it's simplier, hasEnoughAllowance already includes result of permit checks

@@ -43,7 +45,7 @@ export function getOrderParams(
partiallyFillable: order.partiallyFillable,
sellAmount,
balance,
allowance,
allowance: getBiggerAmount(allowance, permitAmount),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I take allowance value from blockchain OR permit value from order appData, depending on what is bigger

Copy link
Contributor

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

Works good now

}, {} as { [key: string]: true })
return acc
},
{} as { [key: string]: true },
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, you can also specify the type without the casting selectedOrders.reduce<[key: string]: true>

@anxolin anxolin added the RELEASE Included in the release that is being closed label Dec 17, 2024
@anxolin anxolin self-requested a review December 17, 2024 15:48
@vercel vercel bot temporarily deployed to Preview – cosmos December 17, 2024 15:56 Inactive
@vercel vercel bot temporarily deployed to Preview – sdk-tools December 17, 2024 15:56 Inactive
@vercel vercel bot temporarily deployed to Preview – widget-configurator December 17, 2024 15:57 Inactive
@vercel vercel bot temporarily deployed to Preview – cowfi December 17, 2024 15:57 Inactive
@vercel vercel bot temporarily deployed to Preview – explorer-dev December 17, 2024 15:57 Inactive
@vercel vercel bot temporarily deployed to Preview – swap-dev December 17, 2024 15:57 Inactive
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
RELEASE Included in the release that is being closed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No 'Insufficient allowance' warning for permittable tokens in Safe
3 participants