edited by fragosti

Discussion in: #25
In addition to the changes listed below, we are planning to use Swagger to define the REST API once it is finalized.

Changes

REST API

/fees has been renamed /order_config and returns senderAddress in addition to previous fields. Also it's a GET now since it doesn't modify state.

metaData has been added to all order responses (and the actual order has been nested). This is meant to provide a standard place to put non-standard or optional fields.

networkId has been added as a query param to all requests. Defaults to 1.

The v0/v1/v2/... versioning convention has been explicitly outlined.

Pagination now requires a total, per_page, page and records in the response for all collections .

Non order-specific query params (ie. fields that are found in the order object) have been added to the /orders endpoint. Named them "filter params". These query params are makerAssetType, takerAssetType, makerAssetAddress, takerAssetAddress. Without these, you would not be able to express "I want all ERC20 tokens" for example, which seems useful. Open to discussion here.

Added a fee_recipients endpoint that returns a list of fee receiving addresses for a relayer.

Websocket API

Also has the remainingFillableAmount change.

API is now meant to be used only for updates. No more "snapshot" option. If you want a snapshot of the orderbook you should use the REST API.

The "update" event no longer only notifies about new orders, but also any change to the remainingFillableAmount value.

Subscribe channel also should support "filter params", in addition to only makerAssetData, takerAssetData, and traderAssetData order query params. Open to discussion here.

As we now support multiple proxies, the identifier of which proxy to use for the token transfer must be encoded, along with the token information. Each proxy in 0x v2 has a unique identifier. If you're using 0x.js there will be helper methods for this encoding and decoding.

The identifier for the Proxy uses a similar scheme to ABI function selectors.

This comment has been minimized.

* assetDataA=&assetDataB [string]: these are assetData fields. Returns asset pairs that contain assetDataA and assetDataB (in any order). Setting only assetDataA or assetDataB returns pairs filtered by that asset only (optional)

This comment has been minimized.

Hm... these only seem to make sense for fungible assets... For non-fungibles, minAmount & maxAmount would always both be 1, and precision would always be 0.

Maybe we need something similar but slightly different for non-fungibles? I feel like people would be interested in knowing which non-fungibles are tradeable, and against what fungible tokens. This might be out-of-scope for SRA... Thoughts?

This comment has been minimized.

Yeah, this was a recurring problem in discussion (things that make sense for fungibles are kind of clunky for NFTs). IMO, this isn't terrible as it's at least compatible with NFTs (using the method you described), if redundant.

Re: knowing trading pairs, isn't this accomplished by just calling asset_pairs? Are you thinking of some way to parameterize calls by AssetProxy to return e.g. all ERC-20 token pairs or all ERC-721 pairs? Sorry if I'm missing something.

This comment has been minimized.

I know there are some organizations that pull liquidity by querying for token pairs, iterating over the token pairs and querying the orders / orderbook API for each token pair. Getting rid of the token pairs API completely would make it hard to adjust those implementations.

This comment has been minimized.

Relayers have full discretion over the orders that they are willing to host on their orderbooks (e.g what fees they charge, etc...). In order traders to discover their requirements programmatically, they can send an incomplete order to this endpoint and receive the missing fields, specifc to that order. This gives relayers a large amount of flexibility to tailor fees to unique traders, trading pairs and volume amounts.

This comment has been minimized.

It'll be a separate request for each order that changed? Maybe we could make this an array? Not sure about the efficiency gains to this, but it's highly likely that several orders get updated at once (e.g when a block gets mined), so might make sense.

This comment has been minimized.

*`bids` - array of signed orders where `takerAssetData` is equal to `baseAssetData`

*`asks` - array of signed orders where `makerAssetData` is equal to `baseAssetData`

Bids will be sorted in descending order by price, and asks will be sorted in ascending order by price. Within the price sorted orders, the orders are further sorted first by total fees, then by expiration in ascending order.

This comment has been minimized.

Total fees seems like an odd thing to sort by. If I have Order A offering 400 ZRX for 1 WETH with a fee of 1 ZRX, and Order B offering 4000 ZRX for 10 WETH with a fee of 5 ZRX, the fee per unit is better on Order B even though the fee for the whole order is higher.

Also, total fee doesn't seem relevant to the taker of the order (and they're the one running the search). It seems to me that they'd only be interested in the taker fee. So if Order X has a taker fee of 1 ZRX and a maker fee of 0, while order Y has a taker fee of 0 and a maker fee of 5 ZRX, the taker is going to be more interested in Order Y where they will pay no fees.

With SRA v1, OpenRelay kind of deviated from this rule, and sorted by what we called "taker fee price", which was essentially the taker fee divided by taker token amount, which would offer the orders that are the best deal for the taker first.

This comment has been minimized.

edited

Contributor

@AusIV this is great feedback! This part of the spec was copied directly from the previous version of SRA but I like the "taker fee price" concept and will include it in the new spec unless anyone else has any objections.

This comment has been minimized.

I had a thought here, could we make this field "orderInfo" instead of "remainingFillableAmount" order Info would simply be the output of doing a "getOrdersInfo" call to the exchange contract. Hydrating this information would require one rpc call

This comment has been minimized.

Can we clarify that this is with respect to the takerAssetAmount? I feel that it could be interpreted takerAssetAmount or makerAssetAmount, and I'm just assuming it's takerAssetAmount because of how the contract internals are implemented.

This comment has been minimized.

I'm wondering if remainingFillableTakerAmount should be nested within an optional field. I wouldn't expect all relayers to include this information since technically all validation can be done client side.

This comment has been minimized.

(Echoing my thoughts from the dev meeting)
Marking this as optional definitely seems like the right direction.

It seems a little off to me to include remainingFillableTakerAmount in the SRA because in practice it shouldn't be consumed in production because the data will be unreliable coming from a relayer. Ultimately anyone using this information will have to build a way to get the correct data directly from the blockchain to launch to production.

I definitely agree that it's a good idea to have somewhere for relayers to include any non standard information they would like to, with this as a possible example.

This comment has been minimized.

Unrelated: is this field intended to account for user balances and allowances, or just the amount still available in the order? If the latter, it should be renamed to remainingTakerAssetAmount to be consistent with the contracts . I also have a feeling that the former is too complicated and will not actually be implemented by most relayers.

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.