BankSystem: cached balances

[whatston3]

About the PR

Adds caching to the server's BankAccountComponent handler for ComponentGetState.

Logic should largely follow previous behaviour - only the user controlling a given entity can update its value in the database/cache. Adds a dictionary in the BankSystem (yes, likely poor form, may need a refactor) to prevent unnecessary DB lookups when the last requested value matches the most recently written one (even if the value hasn't gone through into the DB)

Why / Balance

Updating BankAccounts was a pain, and the ComponentGetState function seems to get called not only on writing values but on queries as well.

Should clear up any issues for #1399 - will behave a lot better with many clients.

How to test

1. Deposit money with a character, attach a breakpoint on Content.Server/_NF/Bank/BankSystem.cs:78 or so to ensure that only one DB hit results from the deposit.
2. Disconnect, close the server, reopen it, check your character's money, should be consistent with the deposited value.

Media

• [X] I have added screenshots/videos to this PR showcasing its changes ingame, or this PR does not require an ingame showcase

Breaking changes

Changelog

Largely an internal feature, no changelog needed.

[whatston3]

Still needs work - might write to cache/DB on component write instead of component write, dirty, component read and cache/DB write.

[whatston3]

Rewrote the bank account system. What exists now should work much more simply than it did before.

Balance in the BankAccountComponent is now only available server-side (though this could change, the value itself is unimportant w.r.t. DB state). TryBankDeposit and TryBankWithdraw now query the cached character preferences and update them directly, absolutely no reliance on component values. BankAccountComponentState was removed entirely, and the ComponentGetState handler was removed. The BankAccountComponent's value is updated when players are attached or detached from an entity, or when a deposit/withdrawal is made. One upside of this is that cloning or moving to a new character only means adding a BankAccount to the character.

With a small set of tests, this seems alright. Might have implications with aghosting into/out of urists. Will invalidate prior comments.

[dvir001]

Loadout wont charge, pending fix.

[whatston3]

Issues should be fixed. I've added bank functions without entities for use in the loadout selection. The new API might not be great, and requires some care under use, but it does seem to work under modest testing. The functions should be commented as such.

I've added session to the PlayerSpawningEvent and code that uses it, since we need to know this to get the bank account. A bankless character will spawn without issue, but will receive a default or fallback loadout, no freebies, shouldn't have much potential for abuse if there are very expensive loadout options.

[github-actions[bot]]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[GreaseMonk]

This PR need 3 approvals before merging. Bugs like these can potentially reset banks to 0$ and that would be really really bad

[GreaseMonk]

Please edge case test spawn as diona die and split into mini dionas after reform, check bank

Just as a precaution because this is where bank stuff is also transferred onto new entities.

[whatston3]

Please edge case test spawn as diona die and split into mini dionas after reform, check bank

Just as a precaution because this is where bank stuff is also transferred onto new entities.

I have added two new calls for updates - a new event when the DB's preferences load finishes (this was the issue with reconnects), and a ComponentInit (this handles late-added components - if you remove someone's BankAccountComponent and add it, that works fine now, their balance will be up-to-date with their last cached state).

In terms of bank account safety, this is better than before - the server's BankSystem does not trust any Client's component state ever. With this, Client systems for vending machines, etc. could just attempt to withdraw funds without checking the component's state.

I think this is good to go, so I'll request another look from @GreaseMonk. Cloning's fine, nymph splitting is fine. All cases where a BankAccountComponent has been set up have been altered before his last review - none of these can set the balance now, it is all driven through the BankSystem, which is good.

[github-actions[bot]]

This pull request has conflicts, please resolve those before we can evaluate the pull request.