OR-1419 update onApprove logic #84
Conversation
| PORTAL.depositTransaction(_to, _value, _gasLimit, false, _data); | ||
| } | ||
|
|
||
| /// @notice unpack onApprove data | ||
| /// @param _data Data used in OnApprove contract | ||
| function unpackOnApproveData(bytes calldata _data) public pure returns (address _to, uint32 _minGasLimit, bytes calldata _message) { |
There was a problem hiding this comment.
Thank you always!
About unpackOnApproveData(onApprove), It seems only L1CDM can set the target Address. Is there any reason?
There was a problem hiding this comment.
Thank you!
Because for other function, it can be encoded into the message directly. The target in CDM is used for relayMessage in sendMessage or sendNativeTokenMessage in L1 to relay a function in L2.
There was a problem hiding this comment.
In optimism portal, I keep the same logic as previous version, only deposit native token.
There was a problem hiding this comment.
Oh, Thank you very much. I understood it thanks to your explanation.
@Zena-park, @nguyenzung
How about removing onApprove in CDM? I think there is no receive function in legacy. I'm sorry about I'm missing about this.
And one more, I think we can use onApprove only for send TON same as legacy. (not message) Is it bad idea,,,? 🙇🏼♂️
There was a problem hiding this comment.
I think this function does not really matter if we add or not.
There was a problem hiding this comment.
@rlgns98kr Thank you Steven, let me update it. I also forgot formatting code.
For encoded message, because owner and amount is already available, so should we only have _to, _minGasLimit and _extraData? I think it will reduce redundant checking and gas cost. In general, we only encode which params in the target function but not exist in onApprove()
There was a problem hiding this comment.
Thank you very much!
I think you are correct!
But in my thoughts, we may explain about approveAndCall to the frontend or users in the future. So if we unpack all arguments, we explain that more easily.
Is it okay? Is it a waste of gas..?
There was a problem hiding this comment.
It is okay, although will cost some more gas. Maybe it is easier for the frontend team.
There was a problem hiding this comment.
Thank you, Steven!
I updated
nguyenzung
force-pushed
the
OR-1419-titan-canyon-update-onApprove-for-L1CDM-and-test
branch
from
4850e9c
to
65d6706
Compare
nguyenzung
requested review from
theo-learner,
ohbyeongmin,
rlgns98kr,
AaronLee22 and
0x6e616d
nguyenzung
force-pushed
the
OR-1419-titan-canyon-update-onApprove-for-L1CDM-and-test
branch
from
e9c5c18
to
58fcc6d
Compare
nguyenzung
merged commit e5a7cae
into
OR-1257-Update-smart-contracts-for-deposit-TON-in-L1
Thank you!