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

feat: bulk listing creation from orderbook SDK #1885

Merged
merged 20 commits into from
Jun 18, 2024
Merged

Conversation

Sam-Jeston
Copy link
Contributor

@Sam-Jeston Sam-Jeston commented Jun 11, 2024

Summary

Implement bulk listing creation from the orderbook SDK. When the bulk listing methods are called with single items, the existing prepareListing and createListing methods are delegated to instead, reducing the impact on enriched confirmations for the majority of users who will not be listing in bulk.

Detail and impact of the change

Added

  • orderbook.prepareBulkListings

@Sam-Jeston Sam-Jeston marked this pull request as ready for review June 12, 2024 02:28
frankisawesome
frankisawesome previously approved these changes Jun 12, 2024
Copy link
Contributor

@frankisawesome frankisawesome left a comment

Choose a reason for hiding this comment

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

lgtm, a few thoughts that are not blocking

@@ -9,3 +10,26 @@ export function getOrderComponentsFromMessage(orderMessage: string): OrderCompon

return orderComponents;
}

export function getBulkOrderComponentsFromMessage(orderMessage: string): OrderComponents[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like bespoke and error prone logic here? Just observation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeh undeniably not ideal - but unfortunately there is no real way around it because of how we expose the signable message to callers, rather than using the inbuilt seaport-js methods


const domainData = {
name: SEAPORT_CONTRACT_NAME,
version: SEAPORT_CONTRACT_VERSION_V1_5,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this something we want to put in to a config/env? Especially now that we use it in multiple places, could be easy to forget if we move to 1.6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those constant values are all imported from seaport/constants.ts. If we move versions updating there will modify all the places we create this message

listingParams: orderParams,
});

const signatures = await actionAll(actions, offerer);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks clean 👍

@Sam-Jeston Sam-Jeston requested review from shineli1984 and a team as code owners June 17, 2024 23:43
@Sam-Jeston Sam-Jeston requested a review from a team as a code owner June 18, 2024 00:27
@Sam-Jeston Sam-Jeston closed this Jun 18, 2024
@Sam-Jeston Sam-Jeston reopened this Jun 18, 2024
lfportal
lfportal previously approved these changes Jun 18, 2024
@Sam-Jeston Sam-Jeston added this pull request to the merge queue Jun 18, 2024
Merged via the queue into main with commit ea1483d Jun 18, 2024
12 checks passed
@Sam-Jeston Sam-Jeston deleted the TD-1498-bulk-listings branch June 18, 2024 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants