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

able to set mintFees for lazyClaim #101

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open

Conversation

donpdang
Copy link
Contributor

@donpdang donpdang commented Jan 2, 2025

This PR

We want to be able to set Manifold mint fees dynamically. Use case is for polygon network, MATIC cost is different than ETH so we need to be able to accoun and set the fees however we like.
The changes are:

  • Add setMintFees to LazyPayableClaim. This is gated by adminRequired modifier
  • Add tests
  • Add ability to pause claim initializations

Screenshots (if applicable)

Ticket

https://linear.app/manifoldxyz/issue/UPDATE-THIS-PART

Self-Review

  • Review and comment PR file changes
  • Description provides context for the changes
  • Tests are included for the PR (required on server changes)

CR Notes

QA Steps

  • [ ]

Copy link

github-actions bot commented Jan 2, 2025

LCOV of commit 5fcf8c1 during Test! #472

Summary coverage rate:
  lines......: 58.9% (1920 of 3262 lines)
  functions..: 62.3% (282 of 453 functions)
  branches...: 41.8% (391 of 936 branches)

Files changed coverage rate: n/a

Copy link
Contributor

@nikki-kiga nikki-kiga left a comment

Choose a reason for hiding this comment

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

lgtm!
sidenote - is it possible there's an existing bug with insufficient funds check in _transferFunds that isn't inclusive of the mintfee?

@wwhchung
Copy link
Contributor

wwhchung commented Jan 3, 2025

This is not a good PR. We need to store the mint fee in the claim itself so we don't rug old claims when the fee changes.

@wwhchung
Copy link
Contributor

wwhchung commented Jan 3, 2025

We need to store it efficiently in the claim. How many bits do we have left? Can we be smart and store in gwei instead of wei to reduce the amount of bits we need?

Copy link
Contributor

@wwhchung wwhchung left a comment

Choose a reason for hiding this comment

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

See all my comments.

@wwhchung
Copy link
Contributor

wwhchung commented Jan 3, 2025

  1. We should not pass the fee to constructor, otherwise addresses will be different on different chains
  2. If you are allowing fee changes to the contract, then you will need to store the current fee at time of claim initialization, and you will need to read the fee from the claim
  3. If you are allowing for Make setArweaveHashes function virtual and simplify non-implemented #2, then we need to store the fee as efficient as possible. Maybe in gwei, maybe in some other thing. But we shouldn't exceed the bits that would push the lazyclaim object to another storage slot
  4. If 3 is not possible, maybe you ONLY allow the fee to be set once.

@donpdang
Copy link
Contributor Author

donpdang commented Jan 3, 2025

lgtm! sidenote - is it possible there's an existing bug with insufficient funds check in _transferFunds that isn't inclusive of the mintfee?

hmm I thought _transferFuns is inclusive of the mintFee according to

payableCost += merkle ? MINT_FEE_MERKLE : MINT_FEE;

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

Successfully merging this pull request may close these issues.

3 participants