6 ms·
The code is ... interesting. Smells organic, not designed. Comments are rare but usually useful. Highly coupled. Static methods everywhere. Violates SOLID p
by McGlockenshire 13y ago
The code is ... interesting. Smells organic, not designed. Comments are rare but usually useful. Highly coupled. Static methods everywhere. Violates SOLID principles. Basically, ignores current best practices.
Clearly not designed for any sort of automated testing, which should be the first damn thing you do when there's any sort of money involved. Hell, even when there isn't money involved.
We'd already guessed that last bit, given the previous mentions of them having no testing/staging/QA environment.
- pinaceae 13y agono shit, magic the gathering exchange is not coded like paypal.
- aegiso 13y agoWay too much schadenfreude given Gox's history. Although contrary to popular belief Gox was never a Magic exchange, they were a Bitcoin startup at a time when Bitcoin was not much more than internet lols and pizza deliveries. The first thing you do when hacking together a stupid exchange for a joke e-currency isn't writing unit tests. You just write the code and blast it up on a domain you had lying around for a different project. Cue runaway success, a company sale or two, scaling issues with complicated technology and not much precedent legal or otherwise, and this is what you get. There is nothing at all surprising about this code. And it seems Gox's problems were much more deeply rooted than the subjective non-compliance of their code with "best practices".
- McGlockenshire 13y ago> contrary to popular belief Gox was never a Magic exchange The Wayback Machine disagrees. http://web.archive.org/web/20070701000000*/http://mtgox.com http://web.archive.org/web/20070701000000*/http://mtgox.com Whether it was open as one is unclear, but that was the obvious intent here. Also, remember that the current owners aren't the original owners. This code is wrong in design but new in attempted style, made or majorly updated within the past year or two (based on the language features used).
- wcummings 13y agoMy understanding is that while the domain was originally purchased for a MtG exchange, it was never used for this purpose. I've yet to see proof that a MtG exchange has existed on mtgox.com
- wahnfrieden 13y agoMcCaleb has said by email that MtGox was at least at some point used for trading or buying cards.
- acdha 13y ago> The first thing you do when hacking together a stupid exchange for a joke e-currency isn't writing unit tests. As soon as you write anything where a bug causes money to change hands, even on a laugh, you write some tests.
- LandoCalrissian 13y agoNo kidding, I would be so freaked out not writing tests the second money is involved. I just don't understand why he didn't bother to hire some help when things got bigger.
- acdha 13y agoThat's the part I don't understand – I'd be scared witless at the thought of not having tests for anything moving money out of my accounts. When you have millions in those accounts, even a really expensive developer is cheap insurance…
- afreak 13y agoWhile it may have not been a MTG card trading site, Karpeles was involved in running MMO servers based around Ragnarok Online before running an exchange. His involvement carried through during MTGox's existence until maybe a year or two ago.
- tlrobinson 13y agoIf Mt Gox was ever a MTG card trading site, it would have been before Mark purchased it from Jed McCaleb.
- hnisnotreddit 13y ago> schadenfreude Epicaricacy. because you're speaking english.
- kens 13y agoHow is the exception handling? For instance, if something goes wrong when generating a new private key, could they still attempt the transaction but with a null key? (cf the earlier bug that lost 2609 bitcoins.)
- McGlockenshire 13y agoUnfortunately I don't know enough about the bitcoin innards to give you an informative answer, and the answer isn't clear from a plain reading of the code. The leaked code seems to just be an internal API for twiddling wallets. There doesn't seem to be any logic here for either the txid conflict retry bits or the hot/cold transfer bits.
- kens 13y agoThanks for the reply. If you look at each method that can return failure, and then look at the code that calls these methods, do the callers check for failure or just keep going with the transaction? (This would both indicate the quality of the code and show a path that could generate bad transactions.)
- McGlockenshire 13y agoThere are a few (9?) places where "getNullAddr" is called. I assume this would return what was earlier called a "null key." It returns the requested information, except when the new address could not be inserted into the database, in which case it returns false. Less than half of the calls have any failure checking, and a few of those return false as well, and who knows if the things that call those functions have any error checking...
- parasight 13y agoOne class to rule them all.
- troels 13y agoYes, it doesn't look very pretty. It relatively recent code though, making plenty use of php 5.3+ constructs, so you can't even blame it on legacy code.
- danielweber 13y agoI've written uglier code. But I didn't write it in places where I had highly motivated adversaries trying to break it with the result that many people's life savings would be irrevocably lost if I had a bug.
- snorkel 13y agoI'd be less concerned about code quality and technical best practices and more interested in knowing if Mt Gox had any internal notion of preventing common stock market manipulation tricks such as wash trades and chandelier bidding. I bet a lot of bitcoin startups don't know what those terms mean and thereby shouldn't be operating a finance exchange.
- scotty79 13y agoWhy would you want o stop that? It's like trying to stop kissing your fist before you roll the dice, because other players might be afraid that you are doing magic. All the rules of traditional stock market are designed to prevent scaring of gamers so they won't leave casino. Bitcoin doesn't care about faint hearted gamers.