JoinColony / colonyCDapp

An iteration of the Colony Dapp sporting both a fully decentralized operating mode, as well as a mode enhanced by a metadata caching layer
5 stars 14 forks source link

Ether is disappearing from the Incoming funds list #2787

Closed arrenv closed 1 month ago

arrenv commented 3 months ago

Steps to reproduce

  1. Pay funds to a colony with the chains native token, i.e. Ether with contract address 0x0000000000000000000000000000000000000000. (You can do this by adding a local wallet address to metamask on Ganache network.
  2. Go to the Incoming funds page and accept the Ether incoming funds.
  3. The token Ether will disappear from the list.
  4. No others will dissappear.

Expected behaviour

Actual behaviour

ether-dissappearing

rdig commented 2 months ago

@davecreaser you've been assigned to this for while now, was there any progress ?

davecreaser commented 2 months ago

@rdig Nope, I haven't worked on it at all or looked into it yet, was just in my name since I worked on incoming funds already. I'll start work on it today though to get it out of the way.

davecreaser commented 2 months ago

As discussed in the product sync, this needs a little bit more thought before we tackle it.

Context: The way that we handle native chain token incoming funds is very different from other incoming funds, since they could come in at any time and from anywhere, and when this was first build incoming funds were never intended to be historically saved. For that reason, querying native incoming funds just uses a lambda which gets them directly from the chain, but they're never saved in the database. Even if we wanted to save them in the database, it's difficult to know when to actually create the incoming fund object in the database, and even more difficult to separate them into individual transactions. We need to decide as a team how we want to tackle this, and exactly what the behaviour should be.

arrenv commented 2 months ago

@davecreaser Thanks for the info on this and brining it up.

It would seem that the most straight forward way right now to be able to keep track of these incoming native chain tokens is to listen for the claim event. Which, I understand does not keep a record of each individual incoming payment, but would at least keep a more accurate history of amounts.

If we were to go with this approach initially, do you have any ideas about time frames, in days, weeks, months?

davecreaser commented 2 months ago

Yeah that is the simplest for sure, although still not entirely simple. In essence, it would require a new lambda (or possibly some changes to the existing lambda, but that's not the cleanest solution), a new model in the db with some new graphql, and some code changes. Also it would need to be on a separate feature branch (since the deployment would be made more complicated by the lambda and data changes). I would estimate it at a couple of weeks of work tbh.

arrenv commented 2 months ago

@davecreaser Thanks for the info. That is quite a bit of work for a subpar outcome.

Do you have any thoughts on what the best option would be, and rough time frames for that?

davecreaser commented 2 months ago

I agree it's not ideal right now.. I think really the best thing would be to save this and do this as part of a larger rework of this page as a whole, if that is at all planned. The way it was built from the start never really assumed that things would be saved so it's just not set up that way from the beginning, so any attempt to change that behaviour will require quite some changes.

In the meantime, to avoid confusion, we could either disable the functionality of saving history for the other tokens (simple to do, but a shame that we lose that functionality) or have some form of messaging in this table that native chain tokens currently won't be shown there once they are accepted.

The problem with trying to change the current implementation is that those native incoming funds are not even stored in our database before being accepted, and when there is a query to fetch them there is a lambda which goes directly to the chain to calculate them. If we hooked into the claim event in the block ingestor and stored them in the database there (not a huge change, but a couple of days of work maybe) then we still have the problem of how to query them. We'd need to either make a new lambda or try to change the current one to return a history from the database combined with a calculation from the chain, which is a kind of hacky solution and again would add another couple of days work. On top of that, since the data currently isn't saved at all, we'd need to add a new model to amplify, and make sure that that works properly for what we need, potentially adding more work. There isn't really a simpler solution unfortunately because of how far away the current implementation is from the desired outcome.

It's not really complicated work, and it's definitely work that we can do at some point, but it will take time and will need to be done and planned properly for sure.

arrenv commented 2 months ago

Thanks for the breakdown @davecreaser. We are working on financial reporting which will likely become a part of next quarter. This feature really only started out as a question as to feasibility while scoping that and the new dashboard out.

Although, I think the feature it is still useful as is. So, I would like to go with your notification suggestion.

What are your thoughts on this?

Figma design link - Native token row notification

image

davecreaser commented 2 months ago

@arrenv Yeah this looks really nice. I think it's a good temporary stopgap - I'll work on it this afternoon!