This repository was archived by the owner on Dec 23, 2024. It is now read-only.
Fix: replace USE_ALL_GAS_FLAG constant with OUTGOING_TRANSFER_GAS_LIMIT#137
Closed
almndbtr wants to merge 2 commits intoourzora:reserve-auctionfrom
Closed
Fix: replace USE_ALL_GAS_FLAG constant with OUTGOING_TRANSFER_GAS_LIMIT#137almndbtr wants to merge 2 commits intoourzora:reserve-auctionfrom
almndbtr wants to merge 2 commits intoourzora:reserve-auctionfrom
Conversation
Incorporates feedback from @tbtstl in https://github.com/ourzora/v3/pull/122/files#r804800016
ourzora@5511daf introduced temp gas limit for payouts. This defines a new constant for the outgoing transfer's gas limit given its repeated use.
almndbtr
commented
Feb 12, 2022
| contract ReserveAuctionV1 is ReentrancyGuard, UniversalExchangeEventV1, IncomingTransferSupportV1, FeePayoutSupportV1, ModuleNamingSupportV1 { | ||
| /// @dev The indicator to pass all remaining gas when paying out royalties | ||
| uint256 private constant USE_ALL_GAS_FLAG = 0; | ||
| uint256 private constant OUTGOING_TRANSFER_GAS_LIMIT = 30000; |
Contributor
Author
There was a problem hiding this comment.
@kulkarohan - When this was originally written in 5511daf, was 30000 chosen for a particular reason over another number? I also wonder about the gas limit for handling a royalty payout being set at 200000.
Here to learn! 💭
Contributor
There was a problem hiding this comment.
hey @almndbtr, the numbers are rough estimates based on the gas usages tested for _handleOutgoingTransfer and _handleRoyaltyPayout. we're bumping both up to be safe, as its difficult to predict every edge-case ahead of time. a PR will be incoming soon taking care of this and removing the variable, so feel free to close this. but thanks for bringing it up!
Contributor
Author
There was a problem hiding this comment.
Thank you! Will close now ✌🏽
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Towards #122
This replaces the
USE_ALL_GAS_FLAGconstant with a new constant,OUTGOING_TRANSFER_GAS_LIMIT.Motivation and Context
This addresses two points:
@tbtstl's callout of an unused var ([feat] Reserve Auction v1.0 #122 (comment))How has this been tested?
No tests since these are private constants.
Checklist: