-
Notifications
You must be signed in to change notification settings - Fork 465
Conversation
This comment has been minimized.
This comment has been minimized.
e281cde
to
7eb839e
Compare
4117e4f
to
4403405
Compare
de58499
to
6cd9c1f
Compare
packages/orderbook/src/order_provider/base_sra_order_provider.ts
Outdated
Show resolved
Hide resolved
super(orderStore); | ||
this._httpClient = new HttpClient(httpEndpoint); | ||
this._perPage = perPage; | ||
this._networkId = networkId; |
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.
If networkId
is optional, we should also add a default. Otherwise it could be undefined
.
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.
networkId
is allowed to be undefined in @0x/connect
. Defaulting (to say 1 for Mainnet) could cause confusing behaviour if for example the SRA endpoint is Kovan only (kovan.sra.0x.org) and networkId 1 is provided as a default.
packages/orderbook/src/order_provider/sra_websocket_order_provider.ts
Outdated
Show resolved
Hide resolved
packages/orderbook/src/order_provider/sra_websocket_order_provider.ts
Outdated
Show resolved
Hide resolved
} | ||
const addedRemoved = addedRemovedByKey[assetPairKey]; | ||
// If we have the metadata informing us that the order cannot be filled for any amount we don't add it | ||
const remainingFillableTakerAssetAmount = (order.metaData as any).remainingFillableTakerAssetAmount; |
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.
When would we not have the metadata?
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 do not define what is inside of the metaData
as per the SRA spec. Radar and Launch kit return remainingFillableTakerAssetAmount
but it is not enforced by the spec.
private readonly _map: Map<string, APIOrder>; | ||
constructor(orders: APIOrder[] = []) { | ||
this._map = new Map(); | ||
(this as any)[Symbol.iterator] = this.values; |
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.
Can you add a comment about what this is doing?
|
||
public add(item: APIOrder): void { | ||
const orderHash = utils.getOrderHash(item); | ||
(item.metaData as any).orderHash = orderHash; |
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.
Why do we need this any
cast?
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 do not define what is inside of the metaData as per the SRA spec. Launch kit returns orderHash
but it is not enforced by the spec and Radar does not return 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.
Addressed most of the changes request by @fabioberger; package looks good to me.
2043ec5
to
d45f64a
Compare
Description
@0x/orderbook
Package to help fetch orders from a remote source (Standard Relayer API, Mesh) and keep the local orderbook synced and up to date.
This means
shouldForceRefresh
(in AssetSwapper) is not required as orderbook is kept up to date with the settings provided by the developer. It allows the developer to switch from Polling to Websockets to Mesh with minimal changes.It allows us to modify the architecture of AssetSwapper to transparently refresh quotes with the latest orders prior to execution to reduce transaction failure.
It can be used on Launch Kit to keep a local orderbook in the front end to reduce excess polling and improve performance.
Supported Order Providers:
To be considered at a later date
Checklist:
@0x/orderbook
Fixes #2062 as this will not throw, only returning empty orders.