Introduce custom logic into OnRecvPacket in PFM module to charge fee #520

RustNinja · 2024-05-22T16:21:07Z

Introduce custom logic into OnRecvPacket in PFM module to charge fee

this code in custom pfm inside IbcMiddleware OnRecvPacket method introduced by me.
https://github.com/ComposableFi/composable-cosmos/blob/rustninja/pmf-middleware/custom/custompfm/keeper/keeper.go#L185-L216

all other code in method OnRecvPacket is taken from original version of OnRecvPacket.

#518) all test passed.

…to prevent a spams (#519) All test passed.

blasrodri · 2024-05-22T17:10:06Z

custom/custompfm/keeper/keeper.go

+	}
+}
+
+func (im IBCMiddleware) OnRecvPacket(


don't you need to implement all of the methods?

no, all other methods are implemented because i include into mine IbcMidleware type the original IbcMiddleware that bring implementation of others methods as inherited from original
I just override the only one method to inject my custom fee logic

rjonczy

Why do you target develop2 ?

target: main (current mainnet) or testnet (current testnet)

blasrodri · 2024-05-22T18:02:19Z

Please include an interchain test here

custom/custompfm/keeper/keeper.go

vmarkushin · 2024-05-22T17:44:17Z

custom/custompfm/keeper/keeper.go

+			im.keeper1.Logger(ctx).Error("packetForwardMiddleware error marshaling next as JSON",
+				"error", err,
+			)
+			// return errorsmod.Wrapf(sdkerrors.ErrJSONMarshal, err.Error())


Why we don't return an error here?

This error will be handled by original pfm in case if memo was incorrect.

custom/custompfm/keeper/keeper.go

RustNinja · 2024-05-22T18:52:46Z

Why do you target develop2 ?

target: main (current mainnet) or testnet (current testnet)

this is devnet. first devnet. then will merge into testnet. and will go to testnet.

vmarkushin

Please highlight the changes to the middleware in the code so it's clear where it's the original middleware and where it's our changes

RustNinja · 2024-05-23T17:35:08Z

Please highlight the changes to the middleware in the code so it's clear where it's the original middleware and where it's our changes

at the description

i duplicate here too
https://github.com/ComposableFi/composable-cosmos/blob/rustninja/pmf-middleware/custom/custompfm/keeper/keeper.go#L185-L216

vmarkushin · 2024-05-23T17:59:16Z

custom/custompfm/keeper/keeper.go

+		retries = im.retriesOnTimeout1
+	}
+
+	// im.ibcfeekeeper.Transfer()


Remove such commented code everywhere, otherwise leave a comment explaining why it's needed

vmarkushin · 2024-05-23T18:02:34Z

x/ibctransfermiddleware/keeper/keeper.go

+	Receiver sdk.AccAddress
+}
+
+func (k Keeper) ChargeFee(ctx sdk.Context, msg *ibctypes.MsgTransfer) (*BridgeFee, error) {


Why ChargeFee has something to do with timeouts?

I will rename function
thanks

vmarkushin · 2024-05-23T18:04:21Z

x/ibctransfermiddleware/keeper/keeper.go

+				return nil, fmt.Errorf("token not allowed to be transferred in this channel")
+			}
+
+			minFee := coin.MinFee.Amount


Does this function takes all the fees or only some specific ones?

function
calculate fee based on config as percentage fee + min fee for some specific demon

Is this fee something different? Can't we use only it?

https://github.com/ComposableFi/composable-cosmos/blob/rustninja/pmf-middleware/custom/custompfm/keeper/keeper.go#L167

No. this fee is the logic from original pfm.
it is different.

our requirements is to add extra field depends on channel and denom. where each fee contains 3 params
account where to send this fee, percentage from amount + min amount equal to 20$.

christianvari · 2024-05-27T09:23:41Z

x/ibctransfermiddleware/keeper/keeper.go

+			if priority != nil {
+				p := findPriority(coin.TxPriorityFee, *priority)
+				if p != nil && coin.MinFee.Denom == p.PriorityFee.Denom {
+					minFee = minFee.Add(p.PriorityFee.Amount)
+				}
+			}


If the sender specifies in the memo a priority coin that is not in the TxPriorityFee or with a Denom different from the MinFee one, it will be not charged and no error raised. Is this intentional?

Yes. So FE is responsible to set a proper priority (high, medium, low) depends on config in storage for this denom.

christianvari · 2024-05-27T09:35:59Z

custom/custompfm/keeper/keeper.go

+		if result.Fee.Amount.LT(token.Amount) {
+			token = token.SubAmount(result.Fee.Amount)
+		} else {


result.Fee is calculated on PacketCoin which is token deducted by the pfm fees.
However since here we deduct token instead of PacketCoin and then pass it to ForwardTransferPacket which applies pfm fees to it, we will get inaccurate calculations for non-fixed fees

It depends how to specify the order of charging a fee.
charge bridge fee when pfm already was charged or opposite.

But now problem. i will remove extra code where i intentionally first determinate the pfm fee and use new amount to charge extra bridge fee for specific channel and denom.

I got your point. Accepted your request.

christianvari · 2024-05-27T09:38:29Z

custom/custompfm/keeper/keeper.go

+	if result != nil {
+		if result.Fee.Amount.LT(token.Amount) {
+			token = token.SubAmount(result.Fee.Amount)
+		} else {
+			send_err := im.bank.SendCoins(ctx, result.Sender, result.Receiver, sdk.NewCoins(result.Fee))


In case result.Fee.Amount.LT(token.Amount) we are deducting the Fee from token, however, fees are not sent to the feeAddress and will remain in the receiver account

It is really good point.
Thanks.
Updated the logic.

christianvari · 2024-05-27T10:00:02Z

custom/custompfm/keeper/keeper.go

+			token = token.SubAmount(result.Fee.Amount)
+		} else {
+			send_err := im.bank.SendCoins(ctx, result.Sender, result.Receiver, sdk.NewCoins(result.Fee))
+			if send_err != nil {
+				logger.Error("packetForwardMiddleware OnRecvPacket error sending fee", "error", send_err)
+				return newErrorAcknowledgement(fmt.Errorf("error charging fee: %w", send_err))
+			}
+			ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)})


I'm not sure if the SendCoins here would work correctly. Funds at this point have already been received by the overrideReceiver so they should be transferred from this account probably.
https://github.com/ComposableFi/composable-cosmos/blob/47c2ed65e657e46e2b005de22846f766262484de/custom/custompfm/keeper/keeper.go#L110-L124

@christianvari
thank you a lot for warning.

you are correct. assets already sent to override receiver address. so fee should be sent from that account.
And this is exactly what our custom logic is doing.

Look.

tr := transfertypes.NewMsgTransfer( metadata.Port, metadata.Channel, token, overrideReceiver, metadata.Receiver, clienttypes.Height{ RevisionNumber: 0, RevisionHeight: 0, }, uint64(ctx.BlockTime().UnixNano())+uint64(timeout.Nanoseconds()), memo, ) result, err := im.ibcfeekeeper.GetBridgeFeeBasedOnConfigForChannelAndDenom(ctx, tr)

I created tr variable passing the exact overrideReceiver as a sender
So the result from GetBridgeFeeBasedOnConfigForChannelAndDenom contains a field sender that equal to overrideReceiver.
So all good.
Thank for pointing and double check.

- remove calculation of pfm fee before request calculation of extra birdge fee for channel and denom if exists - fix a bug when send to fee account only one case when fee is less then token amount. so it was a bug. fixed. now send coin always when fee is less then token amount or equal.

Co-authored-by: dzmitry-lahoda <dzmitry@lahoda.pro> Co-authored-by: kienn6034 <kien@notional.ventures> Co-authored-by: rustdev <placex.com@gmail.com> Co-authored-by: rust.dev <102041955+RustNinja@users.noreply.github.com> Co-authored-by: kkast <kkastsevich@gmail.com> Co-authored-by: Kanstantsin Kastsevich <kkast@users.noreply.github.com> Co-authored-by: rjonczy <robert.jonczy@gmail.com> Co-authored-by: tungle <anhletung@notional.ventures>

…-cosmos into rustninja/pmf-middleware # Conflicts: # app/keepers/keepers.go # x/ibctransfermiddleware/keeper/keeper.go

This pr is the same as [this](#520) but into `testnet` branch instead of `develop2`. Test failed in testnet branch bz of prefix migration. tests are fixed in develop2, so once develop2 will be merged to testnet. tests related to prefix will be fixed as well in `testnet` branch.

RustNinja and others added 5 commits May 21, 2024 22:56

added custom pfm middleware to substitue logic on onRecvPacket

fca383d

import custom pfm into app.go (#517)

0188208

use custom pfm instead of original router. all test passed.

28e43dd

clone the logic from original version to custom pfm to introduce fees. (

154f046

#518) all test passed.

Introduce custom logic into OnRecvPacket in PFM module to charge fee …

2b9a785

…to prevent a spams (#519) All test passed.

RustNinja requested review from kkast, blasrodri and joe-bowman May 22, 2024 16:21

remove custom logic in ibc transfer keeper.

cded618

blasrodri reviewed May 22, 2024

View reviewed changes

rjonczy self-requested a review May 22, 2024 17:33

rjonczy suggested changes May 22, 2024

View reviewed changes

vmarkushin reviewed May 22, 2024

View reviewed changes

RustNinja changed the base branch from develop2 to testnet May 22, 2024 18:11

RustNinja force-pushed the rustninja/pmf-middleware branch from 01ef916 to cded618 Compare May 22, 2024 18:28

fix build

b650645

RustNinja changed the base branch from testnet to develop2 May 22, 2024 18:53

RustNinja requested a review from vmarkushin May 23, 2024 13:03

RustNinja mentioned this pull request May 23, 2024

Testnet. Custom PFM. logic into OnRecvPacket to charge fee #521

Merged

vmarkushin reviewed May 23, 2024

View reviewed changes

rename fn and remove commented code.

47c2ed6

RustNinja requested review from vmarkushin and rjonczy May 23, 2024 19:15

christianvari reviewed May 27, 2024

View reviewed changes

RustNinja requested a review from christianvari May 28, 2024 12:07

hoank101 and others added 3 commits June 11, 2024 14:26

Merge branch 'develop2' of https://github.com/ComposableFi/composable…

08a2a6d

…-cosmos into rustninja/pmf-middleware # Conflicts: # app/keepers/keepers.go # x/ibctransfermiddleware/keeper/keeper.go

fix: some issues when update to sdk50

93f377e

kkast approved these changes Jun 11, 2024

View reviewed changes

RustNinja merged commit 8fce893 into develop2 Jun 11, 2024
16 of 17 checks passed

RustNinja deleted the rustninja/pmf-middleware branch June 11, 2024 21:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce custom logic into OnRecvPacket in PFM module to charge fee #520

Introduce custom logic into OnRecvPacket in PFM module to charge fee #520

RustNinja commented May 22, 2024 •

edited

Loading

blasrodri May 22, 2024

RustNinja May 22, 2024 •

edited

Loading

rjonczy left a comment

blasrodri commented May 22, 2024

vmarkushin May 22, 2024

RustNinja May 23, 2024

RustNinja commented May 22, 2024

vmarkushin left a comment

RustNinja commented May 23, 2024

vmarkushin May 23, 2024

RustNinja May 23, 2024

vmarkushin May 23, 2024

RustNinja May 23, 2024

RustNinja May 23, 2024

vmarkushin May 23, 2024

RustNinja May 23, 2024

vmarkushin May 23, 2024

RustNinja May 23, 2024

christianvari May 27, 2024

RustNinja May 28, 2024

christianvari May 27, 2024 •

edited

Loading

RustNinja May 28, 2024

christianvari May 27, 2024

RustNinja May 28, 2024

christianvari May 27, 2024

RustNinja May 28, 2024

Introduce custom logic into OnRecvPacket in PFM module to charge fee #520

Introduce custom logic into OnRecvPacket in PFM module to charge fee #520

Conversation

RustNinja commented May 22, 2024 • edited Loading

Choose a reason for hiding this comment

RustNinja May 22, 2024 • edited Loading

Choose a reason for hiding this comment

rjonczy left a comment

Choose a reason for hiding this comment

blasrodri commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RustNinja commented May 22, 2024

vmarkushin left a comment

Choose a reason for hiding this comment

RustNinja commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianvari May 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RustNinja commented May 22, 2024 •

edited

Loading

RustNinja May 22, 2024 •

edited

Loading

christianvari May 27, 2024 •

edited

Loading