search

sablier-labs / flow

🍃 Smart contracts of the Sablier Flow protocol.
Other
8 stars 0 forks source link

Include both the normalized and the denormalized amount in the transfer events #204

Closed PaulRBerg closed 2 months ago

PaulRBerg commented 2 months ago

In line with https://github.com/sablier-labs/flow/issues/199, we should include both normalized and denormalized amounts in the following events:

Event indexers may be interested in knowing both the transfer amount AND the amount by which the stream's balance has been reduced.

Also, emitting both amounts avoids having to decide which amount to emit. Currently, in the deposit event, we emit the normalized amount, while in the others we emit the denormalized amount; I don't see any objective rationale for why this has to be so.

smol-ninja commented 2 months ago

I agree with this proposal.

Currently, in the deposit event, we emit the normalized amount, while in the others we emit the denormalized amount

The amount to include in the event was decided based on the amount the function takes as an input parameter. In deposit, the input amount is denormalized we emitted a normalized version in the event. Whereas, in the other two, the input is normalized and so we emit denormalized versions.

PaulRBerg commented 2 months ago

Thanks for the additional color.

andreivladbrg commented 2 months ago

based on my answer here, i think we should keep only the amount in token decimals in the events

PaulRBerg commented 2 months ago

I favor that proposal to change the input type of normalizedRefundableAmountOf, but I still find it valuable to emit both amounts in the events. See @smol-ninja's rationale here.

andreivladbrg commented 2 months ago

but I still find it valuable to emit both amounts in the events. See @smol-ninja's rationale here

i’m sorry, but i don’t understand why his point is relevant to the variables emitted in the event. moreover, after agreeing that we will use token decimals (and not 18 decimals) for the amount param in the refund function - i.e. denormalized

smol-ninja commented 2 months ago

The discussion on having two amounts in event was originally discussed in https://github.com/sablier-labs/flow/issues/204.

From it:

Event indexers may be interested in knowing both the transfer amount AND the amount by which the stream's balance has been reduced.

So function params and events are for two different set of parties.

andreivladbrg commented 2 months ago

Should we close this, since we agreed on the fix https://github.com/sablier-labs/flow/issues/208?

PaulRBerg commented 2 months ago

Yes.