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

Remove protocol gas limits #675

Merged
merged 27 commits into from
Jul 6, 2023
Merged

Remove protocol gas limits #675

merged 27 commits into from
Jul 6, 2023

Conversation

anajuliabit
Copy link
Contributor

@anajuliabit anajuliabit commented Jun 8, 2023

Fix #668

@anajuliabit anajuliabit force-pushed the remove-gas-limits branch 9 times, most recently from c28078a to 9f8dd8e Compare June 12, 2023 15:25
@anajuliabit anajuliabit marked this pull request as ready for review June 12, 2023 15:26
@anajuliabit anajuliabit force-pushed the remove-gas-limits branch 2 times, most recently from 89a2629 to f10aa66 Compare June 12, 2023 15:31
@anajuliabit anajuliabit marked this pull request as draft June 12, 2023 15:48
@anajuliabit anajuliabit marked this pull request as ready for review June 13, 2023 11:42
@anajuliabit anajuliabit self-assigned this Jun 13, 2023
@coveralls
Copy link

coveralls commented Jun 13, 2023

Coverage Status

coverage: 99.693% (-0.01%) from 99.704% when pulling 11bfb4d on remove-gas-limits into f14d952 on main.

Copy link
Member

@zajck zajck left a comment

Choose a reason for hiding this comment

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

A few things:

  1. We should keep maxTwinsPerBundle. Length of _bundle.twinIds is not problematic during in createBundleInternal, but later in transferTwins. Transfer twins should limit the gas it passes to twin contract when making a transfer #687 will add a max gas that a single twin transfer can consume, but if there is a large number of twins, they could collectively still reach the block gas limit and effectively DoS redeemVoucher. If we keep maxTwinsPerBundle we can ensure that maxTwinsPerBundle * maxGasPerTwinTransfer will always be low enough that redeemVoucher will succeed.
  2. burnPremintedVouchers(uint256 _offerId) must also accept uint256 _amount, so the seller can regulate how many vouchers are burned. Currently end = range.start + range.minted; can be so high that the gas limit is reached. It should be end = range.start + amount; instead.
  3. [just a remark, nothing to do]. createTwinInternal can run into a block gas limit since the following value can be arbitrarily high.
    uint256 twinRangesLength = twinRanges.length;
    // Checks if token range isn't being used in any other twin of seller
    for (uint256 i = 0; i < twinRangesLength; i++) {

    The problem is alleviated with Fix twin inefficiencies  #656 but we cannot say it won't exist anymore. In theory, a seller could have so many twins, that creating a new twin won't be possible anymore (around 11,500 at current gas limits). We could introduce maxActiveTwins, so if the seller reached it, they could not create a new twin until they remove some of the existing ones. But that check would be part of createTwinInternal again, so the overall behaviour is the same:
    • Without check (as it is now) they run into a block gas limit when creating a twin. If they remove a few of them, they can again create new ones.
    • With a check in place they would get a revert reason instead when creating a twin. But then they would still need to remove a few of them to be able to create new ones.
  4. getAvailableFunds can run into a gas block limit if tokenList gets too long.
    availableFunds = new Funds[](tokenList.length);
    // Get entity's availableFunds storage pointer
    mapping(address => uint256) storage entityFunds = lookups.availableFunds[_entityId];
    for (uint256 i = 0; i < tokenList.length; i++) {

    We need a separate issue to fix it. Probably accepting a _tokenList would do.
  5. [just a remark, nothing to do]. removeTwins can run into a block gas limit since the following value can be arbitrarily high.
    uint256 lastIndex = twinRanges.length - 1;
    for (uint256 index = 0; index <= lastIndex; index++) {

    This is fixed in Fix twin inefficiencies  #656
  6. I agree that scripts to estimate limits are not necessary anymore. But they still might be valuable to get a feeling of how much the protocol can handle. Maybe you can remove them for now, but at some point, they can be refreshed and the docs rearrange to not talk about the limits (as protocol values) but rather just about protocol capabilities.

@anajuliabit anajuliabit requested review from zajck and mischat July 6, 2023 02:21
@anajuliabit anajuliabit requested a review from zajck July 6, 2023 13:56
zajck
zajck previously approved these changes Jul 6, 2023
Copy link
Member

@zajck zajck left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@mischat mischat requested review from mischat and zajck July 6, 2023 15:58
Copy link
Member

@mischat mischat left a comment

Choose a reason for hiding this comment

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

LGTM

@anajuliabit anajuliabit merged commit 7cf35c4 into main Jul 6, 2023
9 checks passed
@anajuliabit anajuliabit deleted the remove-gas-limits branch July 6, 2023 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Protocol - Removing all of the "gas limit checks"
4 participants