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: gas optimization branch #543

Merged
merged 4 commits into from
May 17, 2024
Merged

fix: gas optimization branch #543

merged 4 commits into from
May 17, 2024

Conversation

aaitor
Copy link
Member

@aaitor aaitor commented Apr 18, 2024

Description

The objective of this change would be to reduce the amount of gas used for the NFT1155Subscription contract, specially for the burn mechanism.

Is this PR related with an open issue?

Related to Issue #

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • Follows the code style of this project.
  • Tests Cover Changes
  • Documentation

Funny gif

Put a link of a funny gif inside the parenthesis-->

@aaitor
Copy link
Member Author

aaitor commented Apr 18, 2024

current burn in v3.5.7:
image

@@ -9,8 +9,8 @@ import './NFT1155Upgradeable.sol';
contract NFT1155SubscriptionUpgradeable is NFT1155Upgradeable {

struct MintedTokens {

Choose a reason for hiding this comment

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

Try to squeeze the whole struct such that in summary its fields are <= 256 bits (aka uint256, 32 bytes or one slot). For example, the expirationBlock / mintBlock can be defined as uint64 as (for example, Arbitrum) is a bit faster than 2 blocks per second, and one might calculate for how long this value is safe.

As for the amountMinted, depending on the projected total supply and decimals, this might be bounded by uint96?

After these changes, we have uint64 + uint96 + uint64 + uint8 (bool) < uint256.

Copy link
Member Author

@aaitor aaitor Apr 18, 2024

Choose a reason for hiding this comment

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

The problem of that approach is would break compatibility with existing subscriptions minted. Probably this contract only could be upgraded adding a new mapping but existing subscriptions could not be burned anymore.

@kupermind
Copy link

@aaitor Couple of more things:

  • Cache _msgSender() before using in multiple places, also try to cache anything that is read from storage, for further multiple usage;
  • Get memory array from _tokens[_key] and loop over it. Also note, that you are pushing into the same _tokens[_key] array, and depending on how the compiler treats the _tokens[_key].length, you might parse elements that were just added.
  • Think about the algorithm whether it's possible to reduce the loop size or pre-collect data / use maps?

@aaitor
Copy link
Member Author

aaitor commented Apr 30, 2024

I pushed a new contract without the time expiration feature (so no track of expiring blocks needed). It comes with a significant reduction on gas cost:
image

In the test burn gas cost goes from Avg 129k to 64k with is the half.

This new contract NFT1155SubscriptionWithoutBlocks is adapted to the functionality you needed and don't include the overhead of the blocks control and storage.

@aaitor aaitor marked this pull request as ready for review April 30, 2024 06:44
@aaitor aaitor requested review from a team as code owners April 30, 2024 06:44
@aaitor aaitor merged commit c888373 into main May 17, 2024
3 checks passed
@aaitor aaitor deleted the fix/gas_optimization branch May 17, 2024 13:38
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.

2 participants