omgnetwork / omg-childchain-v2

pronounced /Ch-ch/
Apache License 2.0
5 stars 2 forks source link

Saving fees to the database #101

Closed ayrat555 closed 4 years ago

ayrat555 commented 4 years ago

https://github.com/omgnetwork/childchain/pull/96#discussion_r466887247

FeeServer will be storing fees in the postgres

  1. All instances of the childchain will be saving fees to the postgres. but the value will be updated only if it hasn't been already inserted, it can be based on updated_at field in fees structs or the hash of the whole struct.
  2. Add caching for fees. but the expiration period will be low so we won't have inconsistencies between nodes. I think there is a probability of inconsistency even if the expiration period is low so values can be fetched from the DB without caching.
ayrat555 commented 4 years ago

the conversation is moved from (https://github.com/omgnetwork/childchain/pull/96#discussion_r468144203):

@achiurizo :

I'm all for using plug middlewares, but I think the crux of the issue is ensuring that we don't have stale fees right? I see that @ayrat555 in #101 listed that concern/solution as so: using caching we can somewhat normalize what the fees are across the nodes(of course, need to work out inconsistencies regarding expiry).

If that's the core case for why do this via plug middlewares, then I'd suggest we just do this later in the flow like in a changeset/hook in Ecto(or even background job). Having the fees pulled/loaded into the request cycle like this could leave the request failing if something were to occur with either the fee and/or transaction. So another question follows:

From a UI/UX perspective, do we:

front-load fee behavior into plug/request cycle? If it fails, ask users to resubmit transaction?
back-load fee behaviour after the transaction is persisted? If it fails, we re-queue the job/hook to add fees in later. Should prevent users from having to re-submit.

@ayrat555:

I'll start implementing persisting and caching of fees now and ask for review later today.

I'm for the second approach. The first approach will require some changes from users. But if it's not a problem, I am okay with either way
InoMurko commented 4 years ago

Caching, in terms of quick access of fees should not be our priority right now. First, we need something that works and later we can figure out if it needs to work fast. This will make the initial approach less complex. It's easier to add things on top then to remove.

Your first point, as I was arguing already, should not base on updated_at except if "time" won't leave centralised postgres. Meaning, the whole operation needs to be decided in the postgres transaction by using internal postgres functions like now() to compare it with updated_at. The reason is that your updated_at coming from fee feed or childchain you cannot ensure that all participants in the exchange have the same view of what now means. You need to find another way.

  1. That doesn't make sense. Even low caching periods cause inconsistencies between nodes if you're dealing with TPS that goes into thousands. Caching should be done when it's needed because of performance issues.

This is normally done like so: You keep two instances of fees in the database. The first is used, the second is waiting on it's turn. And you flip the switch so that requests coming after the switch, use the second fees instance while you drop the first one.

ayrat555 commented 4 years ago

@InoMurko I just started implementing persistence of fees to the db. And yes, I found that updated_at can not be used because it belongs to the token, not the whole fees struct.

My current idea is to have in the db a table with fields: -json - fees -hash - hash of the previous field data (primary key) -inserted_at

Nodes will be fetching the latest record by inserted field. It will be inserted with on conflict do nothing

InoMurko commented 4 years ago

Good stuff @ayrat555