-
Notifications
You must be signed in to change notification settings - Fork 169
feat: disable v2 trasanctions for coin-type zeta #4486
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
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| switch { | ||
| case zrc20 == wzetaContractAddress: | ||
| inboundDetails, err = k.getZETAInboundDetails(ctx, receiverChainID, callOptions) | ||
| return types.ErrZetaThroughGateway |
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 slightly different from the inbound option; the inbound causes the cctx to be in an aborted state, but this error prevents the cctx from being created.
We can abort the cctx here as well, bit this approach seems cleaner,
It would be easier to modify the inbound logic to just ignore the tx instead of aborting it (In case we want similar behaviour for both )
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.
i think it depends on how will it look for users, and how easily they can get information why their cctx is aborted or didnt go through in this case
what is the difference from that point of view do you have example?
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.
For deposits, the inbound hash would end up creating an aborted CCTX with the error message
utils.RequireCCTXStatus(r, cctx, crosschaintypes.CctxStatus_Aborted)
require.Equal(r, cctx.CctxStatus.StatusMessage, crosschaintypes.ErrZetaThroughGateway.Error())
For withdrawals created from zEVM no cctx is created
utils.EnsureNoCctxMinedByInboundHash(r.Ctx, tx.Hash().Hex(), r.CctxClient)
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.
Failing a withdraw does emit an event
https://github.com/zeta-chain/zeta-node/blob/88f4df439231873b70352b6b4ae9fbf9cfe4c468/x/crosschain/keeper/evm_hooks.go#L98-L98
I think this was added to track such failied withdraws , however the logic does not work , since after emitting the event we are returning an error ,
This essentially rolls back the logs
Code from ApplyTransaction
if err = k.PostTxProcessing(tmpCtx, signerAddr, *msg, receipt); err != nil {
// If hooks returns an error, revert the whole tx.
res.VmError = errorsmod.Wrap(err, "failed to execute post transaction processing").Error()
k.Logger(ctx).Error("tx post processing failed", "error", err)
// If the tx failed in post processing hooks, we should clear all log-related data
// to match EVM behavior where transaction reverts clear all effects including logs
res.Logs = nil
receipt.Logs = nil
receipt.Bloom = ethtypes.Bloom{} // Clear bloom filter
} else if commitFn != nil {
The only way to persist this event would be to not revert the tx, essentially just return nil
case zrc20 == wzetaContractAddress:
// EmitEvent from here
return nil
But that would cause funds to get stuck on the fungible module address
| Flow | Behavior | User Funds | Track Transaction |
|---|---|---|---|
| V2 ZETA Deposit | SetAbort() - CCTX aborted, no processing | ZETA locked in connector on external chain | Query CCTX by inbound hash to see error message and aborted status |
| V2 ZETA Withdrawal | Transaction reverts | User keeps ZETA on ZetaChain | No on-chain record exists; zetacore logs contain error details |
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.
makes sense, whatever you see as best fit, if we cant make same behavior without big refactor i think what you did initially is fine
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.
I think saving a withdrawal to the state is not a small refactor, as the EVM hook rolls back any TX that returns an error. Rolling back is also good as there is no loss of funds
As for deposits, we have two options: revert or abort. The issue with revert is that even if we revert it, it still goes through the V2 flow. Aborting is safe and also allows us to refund the user using MsgRefundAborted if needed.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4486 +/- ##
==========================================
- Coverage 65.93% 65.89% -0.05%
==========================================
Files 451 453 +2
Lines 27251 27299 +48
==========================================
+ Hits 17969 17988 +19
- Misses 8310 8336 +26
- Partials 972 975 +3
🚀 New features to boost your workflow:
|
Description
How Has This Been Tested?