RevokeCash / revoke.cash

❌ Revoke or update your token approvals
https://revoke.cash
MIT License
674 stars 238 forks source link

Update "Last Updated" date when updating an approval amount #167

Closed rkalis closed 7 months ago

rkalis commented 10 months ago

Currently, the approval amount updates correctly, but the "Last Updated" date stays the same when updating an approval amount. When refreshing the data, the Last Updated updates correctly of course, but we should make sure that it already updates when the action happens.

kamuik16 commented 9 months ago

Currently, the approval amount updates correctly, but the "Last Updated" date stays the same when updating an approval amount. When refreshing the data, the Last Updated updates correctly of course, but we should make sure that it already updates when the action happens.

Hey @rkalis! Is this issue still open for contribution? I would love to work on it.

rkalis commented 9 months ago

Absolutely! PRs are very welcome!

kamuik16 commented 9 months ago

Absolutely! PRs are very welcome!

Can you just give me more context on the issue so I can then work on it? Right now it's not very much clarifying. Thanks!

rkalis commented 9 months ago

Sure thing, I'll try to provide some more context either this week or early next week!

kamuik16 commented 9 months ago

Sure thing, I'll try to provide some more context either this week or early next week!

Sure no problem!

rkalis commented 9 months ago

Here's some more info on the issue:

In the useAllowances hook, we create an onUpdate(allowance, newAmount) function, to be called when a token approval gets updated (or revoked) - see https://github.com/RevokeCash/revoke.cash/blob/master/lib/hooks/ethereum/useAllowances.tsx#L59.

This onUpdate function gets called in the useRevoke hook - see https://github.com/RevokeCash/revoke.cash/blob/master/lib/hooks/ethereum/useRevoke.tsx#L130.

This causes the "amount" of the token approval to be updated. Now we want to update both of these pieces of code, so that we also update the last updated date and transaction hash.

I think the way to do this would be to change the newAmount parameter to an updatedProperties parameter of type Pick<AllowanceData, 'lastUpdated' | 'transactionHash' | 'amount'>. Then in the onUpdate function we just have to update those properties here, and in the useRevoke hook we will need to pass in those properties.

Passing in the new transaction hash is easy, since we already have that by the time we call onUpdate. But getting the actual last updated time is a bit harder since we will have to request the block time. To do so, we'll need to update the waitForTransactionConfirmation function to actually return the transaction receipt. This transaction receipt will contain the block number. We can then request the block timestamp using our blocksDB.getBlockTimestamp helper.

If getting the actual block time is too hard or if it takes too long, we can also "fake" the last updated time, by just using the current system timestamp, but I think using the actual block time is preferred.

Let me know if that makes sense!

ryanpwaldon commented 8 months ago

@rkalis

In the waitForTransactionConfirmation function, the return type for the waitForTransactionReceipt method is Promise<any>. This is because we don't pass the chain type into the PublicClient generic.

For example, if we typed like so: PublicClient<Transport, typeof mainnet>, the return type of waitForTransactionReceipt would be Promise<TransactionReceipt> (which has the blockNumber property).

Of course, this isn't what we want to do, because we could be using any chain. I see two solutions:

  1. Assume the waitForTransactionReceipt return type always has a blockNumber property (I'm guessing this will always be true, but I'm not entirely sure)
  2. Delay this until we upgrade to viem@2.X.X (this version includes type updates, so the return type will be inferred as is)

Any thoughts?

rkalis commented 8 months ago

Thanks for your thoughts! I think (1) is pretty safe to assume, so we can do that for now. Then we can upgrade to use proper typings here when upgrading to viem v2.