Rewriting credit card network bin rules and adding the card networks: JCB and Diners Club #107
Conversation
There was a problem hiding this comment.
It looks like Stripe is using ranges and has a different set of ranges then we have. Should we just adopt Stripe's logic? We'll get some bonus networks, like JCB, as well.
https://github.com/stripe/stripe-ios/blob/master/Stripe/STPBINRange.m
|
Adam and I decided to follow the Stripe-android code for the bin rules since the the bin set is the most similar to the IIN's listed on wiki |
| @@ -1,6 +1,25 @@ | |||
| import Foundation | |||
|
|
|||
| public struct CreditCardUtils { | |||
| static let cvcLengthAmericanExpress = 4 | |||
There was a problem hiding this comment.
are the cvc lengths getting used anywhere?
There was a problem hiding this comment.
no but i thought maybe we'd use them later?
There was a problem hiding this comment.
Maybe, but let's remove them for now and add them back when we need them.
| static let cvcLengthAmericanExpress = 4 | ||
| static let cvcLength = 3 | ||
|
|
||
| static let maxPanLength = 16 |
There was a problem hiding this comment.
according to the iOS code some of the pans for visa are 14, do they not track that in the Android side?
There was a problem hiding this comment.
they only check for diners club and amex, otherwise standard length
There was a problem hiding this comment.
ok, as long as we're consistent I'm fine with it
| @@ -25,15 +41,12 @@ public struct CreditCardUtils { | |||
| } | |||
|
|
|||
| public static func isValidBin(number: String) -> Bool { | |||
There was a problem hiding this comment.
In the android code, I used the determineCardNetwork method to check for validity. This is more readable an extensible than checking for each possible card:
https://github.com/getbouncer/cardscan-android/blob/master/cardscan-base/src/main/java/com/getbouncer/cardscan/base/CreditCardUtils.kt#L125
| @@ -25,15 +41,12 @@ public struct CreditCardUtils { | |||
| } | |||
|
|
|||
| public static func isValidBin(number: String) -> Bool { | |||
| return isAmex(number: number) || isDiscover(number: number) || isVisa(number: number) || isMastercard(number: number) || isUnionPay(number: number) | |||
| return isAmex(number: number) || isDiscover(number: number) || isVisa(number: number) || isMastercard(number: number) || isUnionPay(number: number) || isJcb(number: number) || isDinersClub(number: number) | |||
| } | |||
|
|
|||
| public static func isAmex(number: String) -> Bool { | |||
There was a problem hiding this comment.
Can we get rid of these methods? Instead of specific isNetwork methods, we should simplify the methods using these.
For example, if we are using these methods to set an icon
if (CreditCardUtils.isAmex()) {
setIcon(amexIcon)
} else if (CreditCardUtils.isUnionPay()) {
setIcon(unionPay)
} else if ...
we should instead create a method in CreditCardUtils that returns the desired icon.
switch (determineCardNetwork(number)) {
case VISA -> visaIcon
case AMEX -> amexIcon
...
}
There was a problem hiding this comment.
Centralizing into one determineCardNetwork method also keeps all the prefix logic in one place instead of spread out around multiple methods
There was a problem hiding this comment.
this also lets us more easily deal with prioritizing discover over unionpay when BINs overlap.
| // | ||
|
|
||
| import XCTest | ||
| @testable import CardScan |
There was a problem hiding this comment.
NICE! thanks for adding tests for this!
|
thank you @awushensky for all the feedback! let me fixes these changes |
jaimejiyepark
changed the title
No description provided.