-
Notifications
You must be signed in to change notification settings - Fork 0
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
Auction House Overhaul #12
base: main
Are you sure you want to change the base?
Conversation
function getCreator(address nftAddress, uint256 /* tokenId */ ) external view returns (address) { | ||
// first make sure nftAddress is a contract | ||
if (nftAddress.code.length == 0) return address(0); | ||
|
||
// passthrough for Ownable `owner` function, with more functionality planned in the future | ||
try Ownable(nftAddress).owner() returns (address owner) { | ||
return owner; | ||
} catch { | ||
return address(0); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like it can be easily abused, nothing enforces that the address returned by owner is the creator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would require a custom contract intended to circumvent this right? The other options are to always enforce royalties or to not enforce them. If you always enforce them, math gets kinda tricky for anyone doing exhibitions/collabs or if the royalty address is different from the artist address. If you don't enforce them on the contract, you have to rely on the frontend potentially passing in expected royalties or some other mechanism for determining primary vs secondary sale and paying out royalties. We've gotten push back from people running exhibitions where if royalties are paid on primary (which doesn't really make sense imo), it's too confusing. This felt like the best approach for standard nfts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct, i think its fine as long as the front end isnt relying on it. i mean overall i think that this space needs some work but given theres nothing better i think its okay as is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah the frontend isn't relying on this for anything really. Right now it's mainly a placeholder contract that can be swapped out for something more robust in the future, like a registry wrapper or something.
…ng sure listings don't start in the past.
_payout(payoutReceiver, currencyAddress, remainingValue); | ||
|
||
// transfer nft to recipient | ||
IERC721(nftAddress).transferFrom(address(this), recipient, tokenId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe an over-optimization but if this reverts for whatever reason, should the funds be returned to the bidder rather than paid out? cc @koloz193
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
every other action in this function accounts for errors received.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id say yea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, otherwise funds just get locked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this shouldn't really happen, but just in case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, just realized, if this function reverts, sending the nft back to the seller probably reverts too. So not much we can do to catch it.
TLAuctionHouse
has received a complete overhaulHere is some documentation on the overhaul: