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

Add max mint limit constraint #5492

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

Conversation

yorhodes
Copy link
Member

Description

Drive-by changes

Related issues

Backward compatibility

Testing

Copy link

changeset-bot bot commented Feb 17, 2025

⚠️ No Changeset found

Latest commit: 6b70d28

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

Copy link

codecov bot commented Feb 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.11%. Comparing base (766f506) to head (6b70d28).

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% <ø> (ø)

destinationToken.standard === TokenStandard.EvmHypVSXERC20Lockbox
) {
const bufferCap = await adapter.getMintMaxLimit();
const max = bufferCap / 2n;
Copy link
Contributor

@Mo-Hussain Mo-Hussain Feb 17, 2025

Choose a reason for hiding this comment

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

If the limit has not replenished to the midpoint and is above it i.e we have recently burned more than minted, we would be allowed to mint more than half the buffer cap at this point.

We could instead check if destinationBalance is greater than getMintLimit()

Copy link
Member Author

Choose a reason for hiding this comment

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

@nambrot wants to avoid this situation where the mint limit is decreasing and the latency of message delivery can cause the transfer to appear mintable at dispatch time but not at process time

Copy link
Member Author

Choose a reason for hiding this comment

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

otherwise we would not have made any changes to this adapter

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

3 participants