-
Notifications
You must be signed in to change notification settings - Fork 94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: call onCrossChainCall
when depositing to a contract
#1226
Conversation
} | ||
if parsedAddress != (ethcommon.Address{}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why we need allow parsedAddress == ethcommon.Address{} ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the fallback when no address is read from the message, it only contains data. So to
will not be updated and will remains msg.Receiver
Currently the fallback is to use msg.Asset
but it doesn't seem valid to me since msg.Asset
represents an address on the external chain and not ZetaChain #1234
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, left a minor comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
minor suggest for change
Description
Simplify the method
HandleEVMDeposit
,to
is replaced by the parsed address in the message if any (previously we separated the parsed address into another variable butto
was not used anymore which added more argument to theZRC20DepositAndCallContract
function than needed)Check if
to
is a contract inZRC20DepositAndCallContract
and useDepositZRC20AndCallContract
when it is the case.The PR also introduces a fix for #1234 since depositing the zrc20 into the
asset
address could result into the loss of tokensCloses: #1216
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Include instructions and any relevant details so others can reproduce.
Checklist: