fix(finance): routingNumber now uses real FederalReserveRoutingSymbol from lookup table.

[woldaker]

Fixes #3290

---

finance.routingNumber is now comprised as follows:

Digits 0-3 = Pulled from locales/base/finance/federal_reserve_routing_symbol.ts
Digits 4-7 = Random
Digit 8 = Check digit. Calculated from the previous 8 digits as before.

Added more test conditions to the randomly-seeded finance.routingNumber test.

[netlify]

✅ Deploy Preview for fakerjs ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	16b5aa6
🔍 Latest deploy log	https://app.netlify.com/sites/fakerjs/deploys/67c3c89f0035de00083bd7c7
😎 Deploy Preview	https://deploy-preview-3429.fakerjs.dev
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.97%. Comparing base (f591459) to head (16b5aa6).

Additional details and impacted files

@@           Coverage Diff            @@
##             next    #3429    +/-   ##
========================================
  Coverage   99.97%   99.97%            
========================================
  Files        2811     2813     +2     
  Lines      216972   217149   +177     
  Branches      944      944            
========================================
+ Hits       216917   217094   +177     
  Misses         55       55            

Files with missing lines	Coverage Δ
src/definitions/finance.ts	100.00% <ø> (ø)
...les/base/finance/federal_reserve_routing_symbol.ts	100.00% <100.00%> (ø)
src/locales/base/finance/index.ts	100.00% <100.00%> (ø)
src/locales/base/index.ts	100.00% <100.00%> (ø)
src/modules/finance/index.ts	100.00% <100.00%> (ø)

[matthewmayer]

Let's add a little more detail to the method description

e.g.

"Generates a random routing number."

=>

"Generates a random [ABA routing number](https://en.wikipedia.org/wiki/ABA_routing_transit_number)"

[woldaker]

Done. I used one of the routing numbers from the test snapshot for the example string.

[woldaker]

Also, I forgot to mention... I couldn't figure out how to link the original issue to this PR. Sorry I am a bit new to contributing to public github repos.

[woldaker]

Also, as I'm sure you've noticed, I skipped any form of testing whether the 2nd group of 2 digits at indexes 2-3 actually matched up with one of the numbers listed in the base finance locale. Of course were I to do that, what I'd really be doing is simply checking if listOfFederalReserveRoutingSymbolsFromBaseLocale.includes(routingNumber.substring(0, 4)), but it appeared as though performing that sort of a check in the tests was not really what y'all wanted. I also considered splitting the first set of 2 digits up from the second set of 2 digits (the institution identifier), which if I had gone in that direction I would have encapsulated the FederalReserveRoutingSymbol into an object and then would have needed to instead make src/locales/base/finance/routing_number.ts and then in there do export const federal_reserve_district = [...]; and export const institution_identifier = (federalReserveDistrict: number) => {...}; or something similar, and then it's no longer a simple lookup table anymore, and I thought all of it was just a bit too overkill and was venturing into over-engineering territory, so I decided against that.

Edit: On a site note, it was interesting to find out that validate.js's isAbaRouting() function does not currently perform any validation on the aspects of an ABA routing number that this PR addresses. I looked at its source. All it's really doing is validating the check digit.

Edit 2: After looking at the validator.js source again, I had missed this regex at the top:

^(?!(1[3-9])|(20)|(3[3-9])|(4[0-9])|(5[0-9])|(60)|(7[3-9])|(8[1-9])|(9[0-2])|(9[3-9]))[0-9]{9}$

Which is checking (though not obviously, of course) that the federal reserve district is correct.
The last bit [0-9]{9} is verifying that it's a total of 9 digits long, which is great, but then you have the entire section before that which, after analyzing it further, is checking the first two digits to make sure that it falls under one of these categories:

Quoted from the wikipedia page on ABA routing numbers:

• 00 is used by the United States Government
• 01 through 12 are the "normal" routing numbers, and correspond to the 12 Federal Reserve Banks. For example, 0260-0959-3 is the routing number for Bank of America incoming wires in New York, with the initial "02" indicating the Federal Reserve Bank of New York.
• 21 through 32 were assigned only to thrift institutions (e.g. credit unions and savings banks) through 1985, but are no longer assigned (thrifts are assigned normal 01–12 numbers). Currently they are still used by the thrift institutions, or their successors, and correspond to the normal routing number, plus 20. (For example, 2260-7352-3 is the routing number for Grand Adirondack Federal Credit Union in New York, with the initial "22" corresponding to "02" (New York Fed) plus "20" (thrift).)
• 61 through 72 are special purpose routing numbers designated for use by non-bank payment processors and clearinghouses and are termed Electronic Transaction Identifiers (ETIs), and correspond to the normal routing number, plus 60.
• 80 is used for traveler's checks

So that regex is effectively trying to be as exhaustive as possible, almost like an official spec would, except it's a little bizarre for two reasons:

One is that it is allowing 000000000, which passes even the check digit validation and is definitely not a valid routing number.
Two is the very strange use of (9[0-2])|(9[3-9]). I know, that's a little nit-picky, but what were the authors doing there exactly? Why not just (9[0-9])?

So tl;dr the call to isAbaRouting() in the test does handle the check digit (and the length=9) for us, but the additional check afterward to see if the first two digits fall within the range of 01-12 are still necessary so as to disallow results y'all aren't interested in, like the thrift/legacy numbers which are no longer issued, traveler's checks, and the 00 govt-only ones.

[ST-DDT]

Please move these into the the it() test blocks.

[woldaker]

Okay. I think I did it that way for DRY purposes, as you'd need to re-construct routingNumber (or any other variable used in multiple it() blocks, but that's the only one this applies to here) each time you needed to reference it inside of the it() block, but if that's what you want, that's fine. I don't quite understand what's wrong with doing it the way I did it, but that's fine. If you care to explain I'd be happy to know though.

[ST-DDT]

Vitest uses callbacks (() => {}) to run the tests. And to re-run them.
If the values are declared outside the callback then they wont change for subsequent executions. We run these tests multiple times to ensure they dont pass by chance (or at least reduce the probability).

faker/test/modules/finance.spec.ts

Lines 107 to 109 in f591459

	describe.each(times(NON_SEEDED_BASED_RUN).map(() => faker.seed()))(
	'random seeded tests for seed %i',
	() => {

[ST-DDT]

IMO these two test cases should be joined into one.

[woldaker]

You can see from validator's source that it is already asserting that it is a string. Does that make the first check completely unnecessary? The one argument I can see for saying NO to that is that that would leave the validity of faker.js's tests dependent upon the implementation details of validator.js.

[ST-DDT]

The string check makes the result in the tests easier to grasp even if you dont read the jsdocs of either method.

[woldaker]

Strange. I keep getting this today:

which ultimately ends up failing the preflight:

When I first went to make the changes you had requested, pnpm complained about being updated, so I ended up doing pnpm setup followed by pnpm add -g @pnpm/exe which gave an error saying that I had to remove the existing pnpm.exe first, so I did Remove-Item C:\path\to\my\pnpm.exe and then reran pnpm add -g @pnpm/exe and then it worked. But now it just fails preflight every time.

Any ideas?

[ST-DDT]

The timeouts happen randomly unfortunately. Even if we increase it to 30s.
We arent sure why this happens, but you can ignore the timeouts for now.

As for the pnpm update, you might have to update node to the latest (LTS) version. That solved it for me.
I use corepack enable to manage the pnpm version in use.