-
Notifications
You must be signed in to change notification settings - Fork 25
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
feat(zora): update plugin to be compatible with zora protocol upgrade [BOOST-4411] #502
Conversation
🦋 Changeset detectedLatest commit: 4903e4e The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -406,7 +451,8 @@ export const getFees = async ( | |||
return { actionFee: fee.tokenPurchaseCost, projectFee: fee.mintFee } | |||
} catch (err) { | |||
console.error(err) | |||
return { actionFee: parseEther('0'), projectFee: parseEther('0.000777') } // https://github.com/ourzora/zora-protocol/blob/e9fb5072112b4434cc649c95729f4bd8c6d5e0d0/packages/protocol-sdk/src/apis/chain-constants.ts#L27 | |||
// ! temp change to base fee of 0.000111 until we update for fee calulation | |||
return { actionFee: parseEther('0'), projectFee: parseEther('0.000111') } // https://github.com/ourzora/zora-protocol/blob/e9fb5072112b4434cc649c95729f4bd8c6d5e0d0/packages/protocol-sdk/src/apis/chain-constants.ts#L27 |
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.
we're simulating v1 mints but the sales config will only work for v2 -- this will likely break backwards compatibility
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.
backwards compatibility might not be possible
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.
Havent fully looked into it yet, but I assume the updated zora sdk should have a way to deal with this.
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.
The SDK should provide a solve here - we also do cache this value still I think so the existing boosts shouldn't break and new boosts will fail on the creation simulation.
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.
Does increase the need for updating the SDK though IMO
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.
I'm seeing mints get routed through the new strategy and the Bulk Mint Tx - ZoraMintsManager
I poked around and didnt' see any instances of mints for this strat getting fired against the collection contract directly but I reckon it's safer to add that to the exist addresses short term.
Outside of that some minor comments and nits but the biggest thing to re-investigate is the exist addresses.
|
||
const _tokenId = await getNextTokenId(client, contractAddress, tokenId) | ||
|
||
await checkBytecode(client, contractAddress) |
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.
No need to move this now but I could see this being generally useful - nice little portable helper.
@@ -12,3 +12,5 @@ export const FIXED_PRICE_SALE_STRATS: { [chainId: number]: Address } = { | |||
} | |||
|
|||
export const ZORA_1155_FACTORY = '0x777777c338d93e2c7adf08d102d45ca7cc4ed021' | |||
export const ZORA_TIMED_SALE_STRATEGY = | |||
'0x777777722d078c97c6ad07d9f36801e653e356ae' |
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.
It's nice they're actually deterministically deploying this now
|
||
return { | ||
chainId, | ||
to: getExitAddresses(chainId, ZORA_TIMED_SALE_STRATEGY), |
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.
It does look like it's still possible to go through 0x777777722D078c97c6ad07d9f36801e653E356Ae
in addition to 0x777777722D078c97c6ad07d9f36801e653E356Ae
and the actual collection.
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.
Hmmmmmmmm - it looks like this address is ZoraTimedSaleStrategy
on OP and ZoraMintsManager
on Zora?
Updates plugin to use the new sales strategy config (0.000111 base fees)
This will only work for free mints at the moment. Will follow-up with update to get the fee calculations working correctly. Fee calculations requires us to update the Zora-sdk which introduces a whole bunch of breaking changes.
Everything is working in local environment except validation. I dont think it is a plugin issue as the transaction shows as OK when using the transaction checker tool.