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.

Sign up for GitHub

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

Jump to bottom

fix: constrain max mint limit for super XERC20 #5492

Open
wants to merge 7 commits into
base: ccip-warp-route
Choose a base branch
from
Open

fix: constrain max mint limit for super XERC20 #5492

wants to merge 7 commits into from

Conversation

yorhodes
Copy link
Member

@yorhodes yorhodes commented Feb 17, 2025 •
edited
Loading

Description

  • Updates XERC20 collateral warp route adapter for Super XERC20 variant.
  • Introduces a new token standard to represent this.
  • Adds logic for comparing to buffer cap limits. If the mint limit is decreasing, latency of message delivery can cause the transfer to appear mintable at dispatch time but not at process time. Thus we cap the mint limit at the buffer midpoint (where it will stop decreasing).

Related issues

Backward compatibility

Yes

Testing

TODO

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Feb 17, 2025 •
edited
Loading

⚠️ No Changeset found

Latest commit: 7d96132

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@codecov Codecov
Copy link

codecov bot commented Feb 17, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.11%. Comparing base (766f506) to head (7d96132).
Report is 99 commits behind head on ccip-warp-route.

Additional details and impacted files 
@@               Coverage Diff                @@
##           ccip-warp-route    #5492   +/-   ##
================================================
  Coverage            77.11%   77.11%           
================================================
  Files                  109      109           
  Lines                 2163     2163           
  Branches               193      193           
================================================
  Hits                  1668     1668           
  Misses                 474      474           
  Partials                21       21
Components Coverage Δ
core 87.80% <ø> (ø)
hooks 77.93% <ø> (ø)
isms 81.60% <ø> (ø)
token 91.66% <ø> (ø)
middlewares 79.80% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

nambrot
nambrot reviewed Feb 18, 2025
View reviewed changes
typescript/sdk/src/warp/WarpCore.ts
destinationBalance = await adapter.getMintLimit();

if (
destinationToken.standard === TokenStandard.EvmHypVSXERC20 ||
Copy link
Contributor

Choose a reason for hiding this comment

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

How will this standard get set? Right now, it is not yeah? wouldn't we need a corresponding change in the warp core config artifact?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, the config artifact is only in the UI afaik?

Copy link
Member Author

Choose a reason for hiding this comment

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

this doesnt need to be urgently merged as @Xaroz already made the change on the UI
this just protects against onchain limit changes

Copy link
Member Author

Choose a reason for hiding this comment

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

I have asked @Xaroz to make this change and test with the UI.

Copy link
Contributor

Choose a reason for hiding this comment

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

tested in our UI and seems to work fine

@yorhodes yorhodes changed the title Add max mint limit constraint Add max mint limit constraint for super XERC20 Mar 5, 2025
@yorhodes yorhodes changed the title Add max mint limit constraint for super XERC20 feat: constraint max mint limit for super XERC20 Mar 5, 2025
@yorhodes yorhodes changed the title feat: constraint max mint limit for super XERC20 fix: constrain max mint limit for super XERC20 Mar 5, 2025
@Xaroz Xaroz mentioned this pull request Mar 6, 2025
Closed
Xaroz and others added 4 commits March 6, 2025 18:10
@Xaroz
chore: add VSXERC20 tokens to getHypAdapter (#5617)
bae1fe1 
### Description

Fix broken test by adding `EvmHypVSXERC20` and `EvmHypVSXERC20Lockbox`
token standards to getHypAdapter
<!--
What's included in this PR?
-->

### Drive-by changes

<!--
Are there any minor or drive-by changes also included?
-->

### Related issues

<!--
- Fixes #[issue number here]
-->

### Backward compatibility

<!--
Are these changes backward compatible? Are there any infrastructure
implications, e.g. changes that would prohibit deploying older commits
using this infra tooling?

Yes/No
-->

### Testing

<!--
What kind of testing have these changes undergone?

None/Manual/Unit Tests
-->
UI test
@Xaroz
feat: rate limit message and refactor
7261ff1
@Xaroz
chore: change check to use MINT_LIMITED_STANDARDS and remove unused a…
8ee8116 
…rray
@Xaroz
feat: rate limit message for MINT_LIMITED Token standards (#5631)
7d96132 
### Description

<!--
What's included in this PR?
-->

Add rate limit message to `validateDestinationCollateral` depending if
the token standard is is part of `MINT_LIMITED_STANDARDS`, I decided to
put this here instead of modifying `isDestinationCollateralSufficient`
because these were used on other places (like the
[UI](https://github.com/hyperlane-xyz/hyperlane-warp-ui-template/blob/fa79e3b4e57460e6995f01eda8b0a74f1e6acd28/src/features/transfer/useTokenTransfer.ts#L121))

### Drive-by changes

<!--
Are there any minor or drive-by changes also included?
-->
Clean up condition check to use less code

### Related issues

<!--
- Fixes #[issue number here]
-->

### Backward compatibility

<!--
Are these changes backward compatible? Are there any infrastructure
implications, e.g. changes that would prohibit deploying older commits
using this infra tooling?

Yes/No
-->
Yes
### Testing

<!--
What kind of testing have these changes undergone?

None/Manual/Unit Tests
-->
UI
![Screenshot 2025-03-07 at 12 48
50 PM](https://github.com/user-attachments/assets/3faf6856-bcdc-46ce-98c3-6002e64c0283)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nambrot nambrot nambrot left review comments

@Mo-Hussain Mo-Hussain Mo-Hussain left review comments

@Xaroz Xaroz Xaroz left review comments

@ltyu ltyu Awaiting requested review from ltyu ltyu is a code owner

@paulbalaji paulbalaji Awaiting requested review from paulbalaji paulbalaji is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
Hyperlane Tasks
Status: In Review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yorhodes @nambrot @Mo-Hussain @Xaroz