Conversation
|
@TerrickMansur Could you pull the latest changes and resolve conflicts, please? |
trsathya
left a comment
trsathya
left a comment
There was a problem hiding this comment.
Hey @TerrickMansur Really appreciate your efforts in adding feature to this fairly new repo. If it is not too much to ask, could you also add getOpenOrders method in other exchanges, where available, please?
| import Foundation | ||
|
|
||
| public enum OrderType: String { | ||
| case Buy = "Buy" |
There was a problem hiding this comment.
String enum values are implicit. No need to specify explicitly unless the value is different from enum case name.
| } | ||
| } | ||
|
|
||
| public class CryptopiaOrder: Order { |
There was a problem hiding this comment.
Rename CryptopiaOrder to just Order. Reason: It is already inside the struct, Cryptopia.
Also, Subclass Order from Cryptex.Order to avoid name collision after renaming.
| self.remaining = NSDecimalNumber(json["Remaining"]) | ||
| self.total = NSDecimalNumber(json["Total"]) | ||
| if let market = json["Market"] as? String { | ||
| let split = market.components(separatedBy: "/") |
There was a problem hiding this comment.
Use convenience init method from CurrencyPair extension in Cryptopia.swift to create CurrencyPair in Order init to avoid redundant code.
| static let USDT = Currency(name: "Tether USD", code: "USDT") | ||
| static let ETC = Currency(name: "Ethereum Classic", code: "ETC") | ||
| static let BCH = Currency(name: "Bitcoin Cash", code: "BCH") | ||
| static let DOGE = Currency(name: "Dogecoin", code: "DOGE") |
There was a problem hiding this comment.
Could you please remove these altcoin currency static members, since I've already added several of them in 0.0.4? Also, I would like to name them using the name instead of their code. Like bitcoin instead of BTC. (I know USDT is a typo there. Will rename it as USDTether in future release, Also, NEM is an exception there)
| case Sell = "Sell" | ||
| } | ||
|
|
||
| public protocol OrderProtocol { |
There was a problem hiding this comment.
Move OrderProtocol and Order class into its own file, say, OpenOrder.swift
| var rate: NSDecimalNumber? { get } | ||
| } | ||
|
|
||
| open class Order: OrderProtocol { |
There was a problem hiding this comment.
Rename Order to OpenOrder. Reason: To distinguish between completed Orders.
| if apiType.checkInterval(response: store.openOrdersResponse) { | ||
| completion(.cached) | ||
| } else { | ||
| cryptopiaDataTaskFor(api: apiType) { (json, response, error) in |
There was a problem hiding this comment.
When you pull the new changes, you would see the new ResponseType as the parameter of dataTask completion block here. Could you fix this?
|
Hey trsathya, Thanks for the feedback. I will work on them and update the branch. I have more work in the pipeline that i will be submitting soon. Cheers, |
|
Hey @TerrickMansur (not trying to be pushy 😄 ) But, just want to say hi and ask about how the PR is coming along. |
2 participants
Creates an OrderModel and OrderType enum. Adds openOrders to store. Implements getOpenOrders for Cryptopia.