-
Notifications
You must be signed in to change notification settings - Fork 56
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
feat: finalize nodev20 branch #388
base: node/v20.0.2
Are you sure you want to change the base?
Conversation
|
||
interface INotSupportedMethods { | ||
error CallOnRevertNotSupported(); | ||
error ZETANotSupported(); |
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.
is there something in GatewayEVM to revert with this?
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.
What do you mean?
We should prevent using callOnRevert
and ZETA functions on EVM side as well
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.
could you please clarify which functions should be disabled on evm side in context of zeta?
deposit and call are either erc20 or gas, which both deposit zrc20
should we disable using zeta connectors on evm side?
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.
should we disable using zeta connectors on evm side?
Yes
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 these are minimal changes for that 9892e63
no need to add reverts in zeta connector since it wont be called from protocol, but even if called, these transfer methods back to connector will revert
on external deposit method using zeta token it will revert as well
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## node/v20.0.2 #388 +/- ##
===============================================
Coverage ? 83.19%
===============================================
Files ? 7
Lines ? 369
Branches ? 124
===============================================
Hits ? 307
Misses ? 61
Partials ? 1 ☔ View full report in Codecov by Sentry. |
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
|
||
interface INotSupportedMethods { | ||
error CallOnRevertNotSupported(); | ||
error ZETANotSupported(); |
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.
What do you mean?
We should prevent using callOnRevert
and ZETA functions on EVM side as well
if (token == zetaToken) { | ||
revert ZETANotSupported(); | ||
// transfer to connector |
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.
If we revert in this case, why do you leave the code below?
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.
doesnt make much difference, to make as minimal changes as possible i guess but can at least comment it out or remove it
gateway = IGatewayEVM(gateway_); | ||
tssAddress = tssAddress_; | ||
_grantRole(DEFAULT_ADMIN_ROLE, admin_); | ||
_grantRole(PAUSER_ROLE, admin_); | ||
_grantRole(PAUSER_ROLE, tssAddress_); |
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.
Is it intentional that TSS becomes a pauser here (can also unpause)? Wouldn't the pausing be intended to save the system in case, for example, TSS becomes corrupted? Because of this, TSS can just unpause itself again and continue draining.
backports and fixes listed in issue
relevant backports from these 2 PRs:
fix: add pauser role to tss #386
fix: review fixes #383
backport for onCrossChainCall rename: refactor: rename onCrosschainCall and zContext #349