Adds network finance system#405
Conversation
|
Let me know when you want this reviewed btw. |
Should be good to review whenever, although I wont be able to address anything til the weekend |
|
After looking at things a bit, I was wondering. Are you planning on porting this upstream? Because it just overwrite a lot of the default economy stuff? |
PsyCommando
left a comment
There was a problem hiding this comment.
There's some things I left comments on, but they're mostly minor things. So I'll approve this for now.
|
|
||
| var/adjustment = 0 | ||
|
|
||
| /datum/controller/subsystem/money_accounts/fire(resumed = FALSE) |
There was a problem hiding this comment.
I gotta admit, I'm not all that fond of holding all account data in-game, and just looping through all of them constantly.
The way I handled them in my rework was via SQL tables and stored procedures. So DM had very little work to do.
But if you really wanna go that way I'm not gonna protest.
There was a problem hiding this comment.
I can see the benefit of that, but since this is looping relatively rarely and performing very few actions, I'm not that worried about performance. I'm also leery of having features which require a working database to function, and that probably wouldn't pass muster upstream.
| var/datum/money_account/max_profit = all_money_accounts[1] | ||
| var/datum/money_account/max_loss = all_money_accounts[1] | ||
| for(var/datum/money_account/D in all_money_accounts) | ||
| if(SSmoney_accounts.all_glob_accounts.len) |
There was a problem hiding this comment.
Ideally use length() instead of len.
There was a problem hiding this comment.
Will be fixed, as other instances.
| var/account_type = ACCOUNT_TYPE_PERSONAL | ||
| var/currency | ||
|
|
||
| var/list/pending_modifications = list() // Modifications pending for the account. |
There was a problem hiding this comment.
Might be a good idea to make this a lazy list?
| source_name = get_source_name() | ||
|
|
||
| target = null | ||
| source = null |
There was a problem hiding this comment.
Won't this cause issues if anything calls valid() on this after perform()?
It seems to me the handling of refs for transactions is making some pretty big assumptions. There's a small timeframe when you essentially make the transaction datum unusable. (Because target and source are nulled out, and everything expects them to be set.)
There was a problem hiding this comment.
This is correct, and for the most part intentional. Once a transaction is finalized, you should never need to check if its valid again. It only remains for logging purposes. I'd say that calling valid() after perform() is always invalid.
| if(prob(50)) | ||
| if(all_money_accounts.len) | ||
| winner_account = pick(all_money_accounts) | ||
| if(SSmoney_accounts.all_glob_accounts.len) |
Description of changes
Adds computer network finance system.
Related Accounts
Network finance
Escrow accounts and Money Security
There are probably other miscellaneous features which cannot be detailed in full, since this is extremely long already. Happy to answer questions on Discord, though.
Authorship
Myself