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

chore: preconfigured orderbook #198

Merged
merged 1 commit into from
Jan 26, 2024
Merged

chore: preconfigured orderbook #198

merged 1 commit into from
Jan 26, 2024

Conversation

mfw78
Copy link
Contributor

@mfw78 mfw78 commented Jan 23, 2024

This PR:

  1. Modifies the PollParams type to take an already-instantiated OrderBookApi.

NOTES: As this is a change to the type, this is a breaking change and is accompanied by a major version bump.

@mfw78 mfw78 requested review from anxolin and a team January 23, 2024 11:39
@mfw78 mfw78 self-assigned this Jan 23, 2024
@coveralls
Copy link
Collaborator

coveralls commented Jan 23, 2024

Coverage Status

coverage: 78.826% (-0.2%) from 78.995%
when pulling 19788dc on preconfigured-orderbook
into 426bea6 on main.

src/composable/types.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

I would not overcomplicate the interface with too many options (config or class instance feels overkill).

Also. If possible, we should work with interfaces and not actual class implementation.

src/composable/ConditionalOrder.ts Outdated Show resolved Hide resolved
src/composable/types.ts Outdated Show resolved Hide resolved
.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

Great there's no TS casting!

Nice also we pass the orderbook API. I think is better than depend on the config.

On my last review, i was hinting that ideally we would depend on simpler interfaces instead of a full instance of a class.

This is a nit, but as you asked for feedback on this regard, I recommend to not depending on the poll method on the full Orderbook API (and less on an instance of an implementation).

You could depend on a simple interface: i.e. getOrder
Or even better, you can close even further the interface by passing a existsOrder(string: orderId): boolean

.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
@@ -1,6 +1,6 @@
{
"name": "@cowprotocol/cow-sdk",
"version": "4.1.0",
"version": "5.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

will this affect to anything in learn.cow.fi?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no use of polling yet in any tutorials there.

src/composable/types.ts Outdated Show resolved Hide resolved
Signed-off-by: mfw78 <mfw78@protonmail.com>
@mfw78 mfw78 changed the title feat: preconfigured orderbook chore: preconfigured orderbook Jan 25, 2024
Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

Soft approve

@mfw78 mfw78 merged commit bc5dd33 into main Jan 26, 2024
7 of 9 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 26, 2024
@mfw78 mfw78 deleted the preconfigured-orderbook branch January 26, 2024 11:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants