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

SRA for 0x v2 #26

Merged
merged 50 commits into from Aug 9, 2018

Conversation

Projects
None yet
8 participants
@dekz
Copy link
Member

dekz commented May 25, 2018

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.
@tomhschmidt

This comment has been minimized.

Copy link
Member

tomhschmidt commented Jul 10, 2018

  • @fragosti to spec out validated order state endpoint
  • Agree on putting out on-chain state changes proposal as optional
  • Agree on networkId as url param
  • @fragosti to think about how to spec per proxy
  • @tomhschmidt to dig into fee recipients endpoint

@fragosti fragosti force-pushed the update/v1 branch from ad15173 to 44c0aac Jul 18, 2018

@fragosti fragosti force-pushed the update/v1 branch from 4e30d16 to 74a3484 Jul 18, 2018

@fragosti fragosti referenced this pull request Jul 18, 2018

Closed

Discussion: SRA for 0x v2 #25

http/v1.md Outdated
curl https://api.example-relayer.com/v1/token_pairs?page=3&per_page=20
```

Page numbering should be 1-indexed, not 0-indexed. Maximum page size should be 100.

This comment has been minimized.

@fabioberger

fabioberger Jul 20, 2018

Contributor

A max page size of 100 seems low. 100 orders takes up 118KB. Let's do research to find a good max page size, still remain conservative but bump this up.

This comment has been minimized.

@AusIV

AusIV Jul 20, 2018

Does the SRA need to be prescriptive about this? Could this not be left to relayers to set a maximum, and have the SRA prescribe a response if the relayer-specific maximum is exceeded?

This comment has been minimized.

@fragosti

fragosti Jul 25, 2018

Contributor

I agree with @AusIV seems - we should just specify the error if the page_size is over their max.

http/v1.md Outdated
{
"field": "networkId",
"code": 1006,
"reason": "Network id 42 is not supported",

This comment has been minimized.

@fabioberger

fabioberger Jul 20, 2018

Contributor

42 is supported. Let's put some other bogus number here for clarity.

This comment has been minimized.

@AusIV

AusIV Jul 20, 2018

42 is supported by the spec, but I would not assume that every relayer will support every network. A relayer that only supports mainnet may return the provided example.

http/v1.md Outdated

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.

@fabioberger

fabioberger Jul 20, 2018

Contributor

Let's link ABI function selectors to educate those who don't know about them.

http/v1.md Outdated
bytes4(keccak256("ERC721Token(address,uint256)"))
```

The encoding for asset data is [ABI encoding](https://solidity.readthedocs.io/en/develop/abi-spec.html).

This comment has been minimized.

@fabioberger

fabioberger Jul 20, 2018

Contributor

Asset data is encoded using ABI encoding.

http/v1.md Outdated
0xf47261b00000000000000000000000001dc4c1cefef38a777b15aa20260a54e584b16c48
```

Encoding the ERC721 token contract (address: `0x371b13d97f4bf77d724e78c16b7dc74099f40e84`), token id (id: 99) and the ERC721 Transfer Proxy (id: 0x08e937fa) would be:

This comment has been minimized.

@fabioberger

fabioberger Jul 20, 2018

Contributor

(id: 99, which hex encoded is 0x63)

http/v1.md Outdated

#### Parameters

* 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.

@fabioberger

fabioberger Jul 20, 2018

Contributor

Why is this optional?

This comment has been minimized.

@AusIV

AusIV Jul 20, 2018

Presumably optional for the sender, not the API implementation. The sender can call it without parameters and get a full (possibly paginated) list of token pairs.

http/v1.md Outdated

* `minAmount` - the minimum trade amount the relayer will accept
* `maxAmount` - the maximum trade amount the relayer will accept
* `precision` - the desired price precision a relayer would like to support within their orderbook

This comment has been minimized.

@fabioberger

fabioberger Jul 20, 2018

Contributor

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.

@tomhschmidt

tomhschmidt Jul 20, 2018

Member

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.

@fragosti

fragosti Jul 25, 2018

Contributor

Do we want to keep this endpoint? Maybe we just have the API return an error when these requirements are not met.

This comment has been minimized.

@AusIV

AusIV Jul 26, 2018

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.

http/v1.md Outdated
* traderAddress [string]: returns orders where either makerAddress or takerAddress has the value specified
* feeRecipientAddress [string]: returns orders where feeRecipientAddress is feeRecipient address

All parameters are optional.

This comment has been minimized.

@fabioberger

fabioberger Jul 20, 2018

Contributor

Do you mean supporting them is optional?

This comment has been minimized.

@AusIV

AusIV Jul 20, 2018

I interpreted that to mean that sending them in an API call is optional.

This comment has been minimized.

@tomhschmidt

tomhschmidt Jul 20, 2018

Member

^ This seems to be a recurring miscommunication :P

Is there a clearer way to distinguish between optional to implement vs. optional to include in the API call?

This comment has been minimized.

@fragosti

fragosti Jul 24, 2018

Contributor

In this spec nothing is optional to implement, so "optional" always refers to the point of view of the user of the API. Of course we can change this but that's the way it's written now.

This comment has been minimized.

@fabioberger

fabioberger Jul 29, 2018

Contributor

Let's just say: All parameters are optional for the caller to supply.

http/v1.md Outdated

### GET /v1/order_config

Submit a partial order and receive information required to complete the order: `senderAddress`, `feeRecipientAddress`, `makerFee`, `takerFee`.

This comment has been minimized.

@fabioberger

fabioberger Jul 20, 2018

Contributor

Let's add a bit more explanation for this one.

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.
ws/v1.md Outdated
},
"remainingFillableAmount": "5000000000000000"
}
}

This comment has been minimized.

@fabioberger

fabioberger Jul 20, 2018

Contributor

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.

@AusIV

AusIV Jul 20, 2018

I'm with @fabioberger here. All of the things that would change the fillability of an order happen at block boundaries, which make it very likely to have more than one at a time.

This comment has been minimized.

@fragosti

fragosti Jul 25, 2018

Contributor

Can't hurt to make it an array

http/v1.md Outdated
"expirationTimeSeconds": "42",
"salt": "67006738228878699843088602623665307406148487219438534730168799356281242528500",
"makerAssetData": "0x01323b5d4c32345ced77393b3530b1eed0f346429d",
"takerAssetData": "0x01ef7fff64389b814a946f3e92105513705ca6b990",

This comment has been minimized.

@AusIV

AusIV Jul 20, 2018

These examples are based on the old format for proxy ID.

http/v1.md Outdated
* `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.

@AusIV

AusIV Jul 20, 2018

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.

@fragosti

fragosti Jul 25, 2018

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.

ws/v1.md Outdated
"makerFee": "100000000000000",
"takerFee": "200000000000000",
"expirationTimeSeconds": "42",
"salt": "67006738228878699843088602623665307406148487219438534730168799356281242528500",

This comment has been minimized.

@AusIV

AusIV Jul 20, 2018

Given the cancelOrdersUpTo feature, we should probably start using salt examples that are timestamps or nonces, rather than huge random values.

This comment has been minimized.

@fragosti

fragosti Jul 25, 2018

Contributor

Will update the examples. Will not call out what the implementation of salt should be (ie. timestamps) as that is called out in the v2 spec https://github.com/0xProject/0x-protocol-specification/blob/master/v2/v2-specification.md#salt

http/v1.md Outdated
"exchangeAddress": "0x12459c951127e0c374ff9105dda097662a027093",
"signature": "0x012761a3ed31b43c8780e905a260a35faefcc527be7516aa11c0256729b5b351bc33"
},
"remainingFillableAmount": "5000000000000000"

This comment has been minimized.

@BMillman19

BMillman19 Jul 31, 2018

Member

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.

@fragosti

fragosti Aug 2, 2018

Contributor

This was discussed in #25 and it seems like people prefer to stick with the single variable.

ws/v1.md Outdated

##### Update

This message is sent whenever the relayer receives a new order, or when the `remainingFillableAmount` of an order changes. The scenarios where an update is necessary include:

This comment has been minimized.

@AusIV

AusIV Aug 1, 2018

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.

@fragosti fragosti changed the title [WIP] SRA for 0x v2 SRA for 0x v2 Aug 2, 2018

http/v1.md Outdated

#### Parameters

Filter parameters:

This comment has been minimized.

@abandeali1

abandeali1 Aug 4, 2018

Member

At the risk of complicating things, all of these filters could be combined into a query with the following params:

  • makerAssetData or takerAssetData
  • a byte range within the assetData (e.g 0-4 for proxy id, 16-36 for address). This would probably be broken down into startIndex and endIndex
  • the target bytes

This is a general way to filter by proxyId, asset address, assetId of a NFT, or any other params in new token standards.

This comment has been minimized.

@abandeali1

abandeali1 Aug 6, 2018

Member

(Discussed in the Aug 6 dev meeting) it is too difficult to index byte ranges of the assetData fields, so we will pass on this proposal for now.

http/v1.md Outdated
"exchangeAddress": "0x12459c951127e0c374ff9105dda097662a027093",
"signature": "0x012761a3ed31b43c8780e905a260a35faefcc527be7516aa11c0256729b5b351bc33"
},
"remainingFillableTakerAmount": "5000000000000000"

This comment has been minimized.

@abandeali1

abandeali1 Aug 4, 2018

Member

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.

@apackin

apackin Aug 6, 2018

(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.

@fragosti

fragosti Aug 6, 2018

Contributor

@apackin thank you for participating in the discussion and yes -- the optionals or meta_data route is the one we are looking to take at the moment.

ws/v1.md Outdated
| 3 | Ropsten |
| 4 | Rinkeby |

Filter parameters (optional):

This comment has been minimized.

@abandeali1

abandeali1 Aug 4, 2018

Member

See above comment re: filter parameters.

* [HTTP](https://github.com/0xProject/standard-relayer-api/blob/master/http/v0.md)
* [WebSocket](https://github.com/0xProject/standard-relayer-api/blob/master/ws/v0.md)

### SRA v1

This comment has been minimized.

@abandeali1

abandeali1 Aug 6, 2018

Member

(Discussed offline) It probably makes sense to 1-index these versions so that they match our contracts.

This comment has been minimized.

@fragosti

fragosti Aug 7, 2018

Contributor

@abandeali1 we should probably keep v0 around. (so go from v0 to v2). A lot of relayers have already namespaced their APIs with v0. Or maybe we can just say that v0 and v1 both point to the same API?

http/v1.md Outdated
"exchangeAddress": "0x12459c951127e0c374ff9105dda097662a027093",
"signature": "0x012761a3ed31b43c8780e905a260a35faefcc527be7516aa11c0256729b5b351bc33"
},
"remainingFillableTakerAmount": "5000000000000000"

This comment has been minimized.

@abandeali1

abandeali1 Aug 6, 2018

Member

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.

This comment has been minimized.

@fragosti

fragosti Aug 6, 2018

Contributor

It's actually intended to be the former, but we could change that. I could move the explanation from the websocket spec @fabioberger under what conditions does OrderWatcher subscribe update?

This comment has been minimized.

@fragosti

fragosti Aug 7, 2018

Contributor

Discussed separately: going to reduce the requirements for this field and move it into some general, catch-all object.

@BMillman19

This comment has been minimized.

Copy link
Member

BMillman19 commented Aug 9, 2018

Looks good, just a couple small things

  • The links to the v2 http / ws specs don't work from the main README
  • We should make a followup issue / ticket to link to the correct json-schemas
@fragosti

This comment has been minimized.

Copy link
Contributor

fragosti commented Aug 9, 2018

@BMillman19 the v2 http / ws specs will work once this is merged. Also, the json-schemas should be up to date once the sra-api branch merges.

@fragosti fragosti merged commit 3487714 into master Aug 9, 2018

@fragosti fragosti deleted the update/v1 branch Aug 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment