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

Errors in the definitions ? #2

Open
StefanoRaggi opened this issue Mar 31, 2017 · 3 comments
Open

Errors in the definitions ? #2

StefanoRaggi opened this issue Mar 31, 2017 · 3 comments

Comments

@StefanoRaggi
Copy link

As a follow-up to my previous issue (#1), I found a few errors in the JSON spec I am using to generate C# bindings. Without the following changes, deserialization errors occurred at runtime.

  1. In PositionSide definition, TradeIds is defined as array of objects instead of array of strings
    https://github.com/oanda/v20-openapi/blob/master/json/v20.json#L5125
    should be: "type": "string",
    instead of: "$ref": "#/definitions/TradeID"

  2. relatedTransactionIDs and closingTransactionIDs are defined as arrays of objects instead of arrays of strings
    https://github.com/oanda/v20-openapi/blob/master/json/v20.json#L480
    https://github.com/oanda/v20-openapi/blob/master/json/v20.json#L11731
    https://github.com/oanda/v20-openapi/blob/master/json/v20.json#L11820
    should be: "type": "string",
    instead of: "$ref": "#/definitions/TransactionID"

  3. OrderPositionFill enum
    https://github.com/oanda/v20-openapi/blob/master/json/v20.json#L3464
    should be: POSITION_DEFAULT
    instead of: DEFAULT
    (also need to update all usages)

  4. OrderTriggerCondition enum
    https://github.com/oanda/v20-openapi/blob/master/json/v20.json#L3471
    should be: TRIGGER_DEFAULT
    instead of: DEFAULT
    (also need to update all usages)

This is the modified JSON file I used for generation: v20.zip

@dmpettyp
Copy link

Thanks again for the feedback.

1 & 2 are being fixed
3 & 4 are actually implementation bugs, not documentation bugs. We will be fixing the implementation to match the documentation

@dmpettyp
Copy link

1&2 should be fixed in the spec now, thanks. 3&4 won't show up until the next release

@StefanoRaggi
Copy link
Author

Thanks for the fixes, do you have an ETA for the next implementation release ?

It would be important to know, because we'll have to wait for it before merging this PR:
QuantConnect/Lean#821

StefanoRaggi added a commit to QuantConnect/Lean that referenced this issue Apr 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants