-
Notifications
You must be signed in to change notification settings - Fork 8
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
Sequential commit #569
Sequential commit #569
Conversation
Bumps [eslint-config-prettier](https://github.com/prettier/eslint-config-prettier) from 8.5.0 to 8.6.0. - [Release notes](https://github.com/prettier/eslint-config-prettier/releases) - [Changelog](https://github.com/prettier/eslint-config-prettier/blob/main/CHANGELOG.md) - [Commits](prettier/eslint-config-prettier@v8.5.0...v8.6.0) --- updated-dependencies: - dependency-name: eslint-config-prettier dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Initalizer Facet documentation * Apply suggestions from code review Co-authored-by: Ana Julia Bittencourt <[email protected]> * Expand initialization description * Review fixes * upgrade configs * Update docs/tasks.md Co-authored-by: Ana Julia Bittencourt <[email protected]> Co-authored-by: Ana Julia Bittencourt <[email protected]>
* operator->assistant * remove console.log * diagram update * Revert assistant back to operator for ERC721/ERC1155
196bd83
to
3d9fb9f
Compare
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.
Lots of questions (informative) + a few remarks.
The most important one is about taking into account the offer collection to retrieve the voucher contract
PriceDiscovery calldata _priceDiscovery, | ||
uint256 _initialSellerId | ||
) internal returns (uint256 actualPrice) { | ||
IBosonVoucher bosonVoucher = IBosonVoucher(protocolLookups().cloneAddress[_initialSellerId]); |
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.
idem above line 92
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.
Fixed.
contracts/interfaces/handlers/IBosonSequentialCommitHandler.sol
Dismissed
Show dismissed
Hide dismissed
* AMM integration tests (WIP) * Make hardhat task work with submodules which uses foundry * Update hardhat config * Fix diamond deploy * Renaming folders * Continue AMM POC * Add price type to offer struct and continue sudoswap poc * Fix interface ids * Moving lssvm submodule * Make boson voucher compatible with price discovery offers * Continue POC work * Finalize AMM POC * Finalize sudoswap POC * Seaport: Create offer with criteria (WIP) * Seaport: advance order with criteria * Move test files * Fix sudoswap tests * Fix seaport tests * Make commitToOffer and commitToPriceDiscoveryOffer a single method * Price discovery POC: auction * Implement new voucher transfer and protocol communication design (WIP) * Removing logs * Fixing compile * Finish new voucher transfer and protocol communication design * SneakAuction (WIP) * Supports bid commits that requires pre-holding the NFT * Token id is mandatory on bid commits * Continue work on PD * Fix submodules hardhat compilation * Finish refactor * Remove SneakyAuction test and add Zora test * Offer can't be price discovery and be transfer externally at the sime time * Add new validations to fulfilBidOrder * Decoupling bid function * Add natspec comments * Fix set committed * Split commitToOffer and commitToPreMintedOffer * Review changes * Apply more review changes * Owner must be priceDiscoveryContract when caller is not voucher owner * Add some others verifications * Fix Zora integration test without wrapper * Fix stack too deep issue by enabling Yul via iR * Split commitToPriceDiscovery in a new PriceDiscoveryFacet * Add weth to protocol protocol addresses storage * Fix interface ids * Creating SudoswapWrraper * Update SudoswapWrapper contract to allow wrapping multiple tokens at once and modify integration test accordingly * Update SudoswapWrapper contract to allow wrapping by sender * Continue SudoswapWrapper * Finalise sudoswap wrapper poc * Adding missing natspec comments * Upgrade hardhat config and minors changes * Fix pre-minted voucher transfer to check for offer price type and transaction origin * update integration tests * update inegration test - sudoswap * Extend priceDiscovery struct * deprecate commitToPremintedOffer * Fix fundsHandler tests * Fix metaTransactionsHandler tests * Fix Offer domain script * Fix sequentialCommitHandler tests * review fixes 1st iter * Correct resellerId in initial exchangeCosts for price discovery * review fixes 2nd iter * More minore fixes in tests/contracts * PriceDiscoveryHandler unit tests * onPremintedVoucherTransferred test + other minor fixes * Zora wrapper (#598) * initial wrapper * Zora auction PoC * code cleanup, add comments * additional auction tests * Review fixes * Remove ownable * review fixes * wrapper tests * custom mock auction * handling wrapper another type of "Side" * Improve wrapper handling --------- Co-authored-by: zajck <[email protected]> Co-authored-by: Klemen <[email protected]>
e0d4660
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.
Slither found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
This PR introduces sequential commit into the protocol.
It also includes methods used for price discovery, since it's needed for tests (
fulFilOrder
,fulfilBuyOrder
andfulfilSellOrder
). This might not be in its final form and might be completely replaced, since at the end we expect to have the same price discovery methods shared betweencommitToOffer
andsequentialCommitToOffer
.Changes:
SequentialCommit
andPriceDiscovery
IBosonSequentialCommitHandler.sol
which implementsIERC721Receiver
interface and has additional methodsequentialCommitToOffer
PriceDiscoveryBase
which:fulFilOrder
,fulfilBuyOrder
andfulfilSellOrder
SequentialCommitFacet
with methodssequentialCommitToOffer
to handle sequential commitsonERC721Received
to handle incoming boson voucherscontracts/protocol/facets/ExchangeHandlerFacet.sol
nonReentrant
modifier was removed fromonVoucherTransferred
contracts/protocol/libs/FundsLib.sol
releaseFundsToIntermediateSellers
to handle funds at finalization time for all involved partiesincreaseAvailableFundsAndEmitEvent
transferFundsToProtocol
andtransferFundsFromProtocol
so they can work without_entityId
Note: this implementation works with the current royalties (one recipient, one percentage). Royalties will be changed in a separate PR and after it's approved, sequential commit will be adapted to work with them.
Removal of reentrancy guard on
onVoucherTransferred
Reentrancy guard on
onVoucherTransferred
prevents voucher transfer during the sequential commit, therefore it was removed. It does not pose a threat to the protocol or vouchers, since it does not make any external calls and it's callable only by voucher contracts, which are under our control. The only reason why we had it here before was to prevent potential inconsistencies in the last field ofVoucherTransferred
. In an edge case scenario,executedBy
can be incorrect - this happens ifaddress1
submits metatransaction to redeem a voucher for offer with twins. If then of the twins executes a boson voucher transfer,onVoucherTransferred
will still showaddress1
as executor and not twin contract address.This is unlikely scenario with no real threat.
There are two alternative approaches that would allow keeping reentrancy guard on
onVoucherTransferred
. One is to modify vouchers to not invokeonVoucherTransferred
when_to
is protocol contract, but that would also require to modifytransferFrom
to not allow unsafe transfers to protocol. Another way would be to modify reentracy guard for this specific case, so it would allow 1 reenter during sequential commit and only toonVoucherTransferred
.However, both approaches are more complex.