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.

Sign up for GitHub

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

Jump to bottom

Premint - if no mint recipient, revert with proper error #312

Merged
merged 1 commit into from
Nov 15, 2023
Merged

Premint - if no mint recipient, revert with proper error #312

merged 1 commit into from
Nov 15, 2023

Conversation

oveddan
Copy link
Collaborator

@oveddan oveddan commented Oct 31, 2023

No description provided.

This was referenced Oct 31, 2023
Merged
Merged
Merged
Merged
Merged
Closed
Merged
Merged
Closed
Closed
@oveddan
Copy link
Collaborator Author

oveddan commented Oct 31, 2023 •
edited
Loading

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

This was referenced Oct 31, 2023
Merged
Closed
Merged
Merged
Merged
Merged
Closed
Merged
@oveddan oveddan force-pushed the allow-minting-on-v1-contracts branch from 44d4d68 to 9b93a9e Compare November 1, 2023 21:37
iainnash
iainnash reviewed Nov 1, 2023
View reviewed changes
Copy link
Collaborator

@iainnash iainnash left a comment

Choose a reason for hiding this comment

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

Just trying to understand the motivation for this pr.

packages/1155-contracts/src/delegation/ZoraCreator1155PremintExecutorImplLib.sol Outdated
@@ -120,7 +120,8 @@ library ZoraCreator1155PremintExecutorImplLib {
uint256 quantityToMint,
IZoraCreator1155PremintExecutor.MintArguments memory mintArguments
) internal {
bytes memory mintSettings = abi.encode(mintArguments.mintRecipient, mintArguments.mintComment);
address mintRecipient = mintArguments.mintRecipient == address(0) ? msg.sender : mintArguments.mintRecipient;
Copy link
Collaborator

Choose a reason for hiding this comment

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

In what case would this be true?

@oveddan
Copy link
Collaborator Author

oveddan commented Nov 1, 2023

Just trying to understand the motivation for this pr.

In case it's not set by accident

@kulkarohan
Copy link
Contributor

hmm i wonder if its better to leave this explicit and let it revert if a recipient isn't passed

@oveddan oveddan force-pushed the allow-minting-on-v1-contracts branch from 9b93a9e to de4d708 Compare November 3, 2023 17:03
@oveddan oveddan force-pushed the allow-minting-on-v1-contracts branch from de4d708 to 8247170 Compare November 3, 2023 18:02
@oveddan oveddan mentioned this pull request Nov 14, 2023
Merged
@oveddan oveddan force-pushed the allow-minting-on-v1-contracts branch from fab7fcc to 58b0ceb Compare November 14, 2023 23:24
@oveddan oveddan mentioned this pull request Nov 15, 2023
Merged
@oveddan oveddan force-pushed the allow-minting-on-v1-contracts branch from 58b0ceb to cbcd00e Compare November 15, 2023 00:45
@oveddan oveddan mentioned this pull request Nov 15, 2023
Closed
@oveddan oveddan force-pushed the allow-minting-on-v1-contracts branch from cbcd00e to 3a22cd0 Compare November 15, 2023 01:10
@oveddan oveddan force-pushed the allow-minting-on-v1-contracts branch from 3a22cd0 to e7fcba1 Compare November 15, 2023 01:12
@oveddan oveddan force-pushed the allow-minting-on-v1-contracts branch from e7fcba1 to 4e5a9e9 Compare November 15, 2023 01:13
@oveddan
Copy link
Collaborator Author

oveddan commented Nov 15, 2023 •
edited
Loading

Merge activity

  • Nov 15, 2:48 PM: @oveddan started a stack merge that includes this pull request via Graphite.
  • Nov 15, 2:48 PM: Graphite couldn't merge this pull request because a downstack PR Premint V2 #262 failed to merge.
  • Nov 15, 4:20 PM: @oveddan started a stack merge that includes this pull request via Graphite.
  • Nov 15, 4:22 PM: Graphite rebased this pull request as part of a merge.
  • Nov 15, 4:23 PM: @oveddan merged this pull request with Graphite.

@oveddan oveddan force-pushed the allow-minting-on-v1-contracts branch from 4e5a9e9 to 83ba3eb Compare November 15, 2023 20:41
@oveddan oveddan changed the base branch from allow-minting-on-v1-contracts to main November 15, 2023 21:21
@oveddan oveddan dismissed stale reviews from IsabellaSmallcombe and iainnash November 15, 2023 21:21

The base branch was changed.

@oveddan oveddan merged commit 597559d into main Nov 15, 2023
9 checks passed
This was referenced Nov 17, 2023
Merged
Closed
Merged
iainnash pushed a commit that referenced this pull request Jan 5, 2024
@oveddan @kulkarohan
Premint - if no mint recipient, revert with proper error (#312)
965ae22
iainnash pushed a commit that referenced this pull request Jan 11, 2024
@oveddan
Premint - if no mint recipient, revert with proper error (#312)
ef6b16c
iainnash pushed a commit that referenced this pull request Jan 11, 2024
@oveddan @kulkarohan
Premint - if no mint recipient, revert with proper error (#312)
dc050c0
@iainnash iainnash deleted the default-msg-sender branch January 11, 2024 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iainnash iainnash iainnash left review comments

@IsabellaSmallcombe IsabellaSmallcombe IsabellaSmallcombe left review comments

@kulkarohan kulkarohan Awaiting requested review from kulkarohan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@oveddan @kulkarohan @iainnash @IsabellaSmallcombe