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

seaport proposal type #30

Merged
merged 9 commits into from
Jun 28, 2022
Merged

seaport proposal type #30

merged 9 commits into from
Jun 28, 2022

Conversation

merklejerk
Copy link
Collaborator

@merklejerk merklejerk commented Jun 15, 2022

seaport listing proposal type

@height
Copy link

height bot commented Jun 15, 2022

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

@merklejerk merklejerk marked this pull request as ready for review June 15, 2022 21:55
@@ -10,6 +10,7 @@ import "../tokens/IERC721.sol";
import "./PartyGovernance.sol";

// ERC721 functionality built on top of PartyGovernance.
// TODO: Just inherit from solmate? 🙄
Copy link
Collaborator

Choose a reason for hiding this comment

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

yes ser

Copy link
Collaborator

@steveklebanoff steveklebanoff left a comment

Choose a reason for hiding this comment

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

very dope. added a few comments

@@ -57,6 +58,21 @@ contract PartyGovernanceNFT is
_;
}

modifier mustOwnTokenOrIsApprovedForAll(uint256 tokenId, address whom) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

def feels like we should just inherit from sol-mate instead of writing this stuff ourselves

@@ -1,14 +1,17 @@
// SPDX-License-Identifier: Apache-2.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

shall we just remove this now? and get rid of shared wyvern maker?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes!

uint256 expiry
);

ISeaportExchange public immutable SEAPORT;
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we be able to support if seaport keeps the same ABI but upgrades to a new address?

also -- what happens if Seaport gets exploited? Should we have a multisig kill switch / escape hatch to disable OS interactions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

imo neither case is worth it. First seems unlikely without some breaking change, second only affects open listings. Also I kind of like that parties have to opt into something like using a new OpenSea version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

fair enough

) {
// Not a unanimous vote and the token is precious, so list on zora
// AH first.
// TODO: Should this be just executionDelay?
Copy link
Collaborator

Choose a reason for hiding this comment

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

it feels reasonable to me for this to be a diff concept than executionDelay

return "";
}

function _listOnOpenSeaport(
Copy link
Collaborator

Choose a reason for hiding this comment

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

seaport is p clean to work with 🙏

orderHash = _getOrderHash(orderParams);
// Validate the order on-chain so no signature is required to fill it.
assert(SEAPORT.validate(orders));
emit OpenSeaportOrderListed(
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice. would like to use this to post this to their api on rinkeby

offer.endAmount = 1;
}
// What we want for it.
orderParams.consideration = new ISeaportExchange.ConsiderationItem[](1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think we need to create two considerations: one for us, and one for an OS fee. worried that they won't accept our order in their API or show it on their site w/o their 2.5% fee.

here's an example of the seaport order that was created for me via their testnet site: https://gist.github.com/steveklebanoff/32331eb2de985206ec7d2f61b814c0ed

@@ -10,6 +10,7 @@ import "../tokens/IERC721.sol";
import "./PartyGovernance.sol";

// ERC721 functionality built on top of PartyGovernance.
// TODO: Just inherit from solmate? 🙄
Copy link
Collaborator

Choose a reason for hiding this comment

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

@merklejerk wdyt about just doing this now?

Copy link
Collaborator

@steveklebanoff steveklebanoff left a comment

Choose a reason for hiding this comment

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

i think once we get thru:

  • use sol-mate instead of custom 721 implementation
  • remove wyvern stuff
  • separate duration and minExpiry w/ diff globals
  • add in ability to add OS fee so we can post to their API

this will be good 2 go

merklejerk and others added 4 commits June 24, 2022 02:42
add openseaport integration test (WIP)
separate zora timeout and duration
fix getProposalState() not recognizing unanimous state.
allow extra seaport fees.
fix tests, get bought seaport order integration test working
refactors
@merklejerk merklejerk merged commit ea7a4bd into main Jun 28, 2022
@@ -728,6 +723,10 @@ abstract contract PartyGovernance is
if (pv.passedTime + gv.executionDelay <= t) {
return ProposalState.Ready;
}
// If unanimous, we skip the execution delay.
if (_isUnanimousVotes(pv.votes, gv.totalVotingPower)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

😇

@0xble 0xble deleted the feat/seaport branch September 26, 2022 16:22
arr00 added a commit that referenced this pull request Nov 30, 2023
* add `SellPartyCardsAuthority`

* update deploy

* add a few tests

* update events and add helper functions

* update relevant abis

* fix tests

* add more tests

* add more tests

* update `isSaleActive` logic

* move Contributed event

* merge `AboveMaximumContributionsError` and `BelowMinimumContributionsError`

* remove redundant initial delegate check

* fix compile errors

* transfer contribution to Party and fix totalContribution

* update `ContributionRouter`

* fix `isSaleActive`

* fix tests and increase coverage

* fix test

* update contribute event

* fix event and tests

* tweaks

* increase coverage

* perf: optimize and refactor `SellPartyCardsAuthority` (#30)

* fix tests

* update delegate comments

* ensure contract is an authority before sale creation

* fix deploy

* emit `Finalized` if not enough room for another contribution

* style: rename `delegate` to `initialDelegate`

* fix(`SellPartyCardsAuthority`): mitigations (#329)

* Fix merge conflict

---------

Co-authored-by: Brian Le <[email protected]>
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