Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(protocol): allow resolver to return zero address for EtherVault #13083

Merged
merged 2 commits into from
Feb 3, 2023
Merged

fix(protocol): allow resolver to return zero address for EtherVault #13083

merged 2 commits into from
Feb 3, 2023

Conversation

dionysuzx
Copy link
Collaborator

this is already checked by the addressResolved because we pass in false to the function. this makes sure it cannot return the zero address.

@dionysuzx dionysuzx requested review from dantaik, cyberhorsey and shadab-taiko and removed request for dantaik, shadab-taiko and cyberhorsey February 1, 2023 21:49
@dionysuzx dionysuzx changed the title enhance(protocol): remove redundant check for zero address refactor(protocol): remove redundant check for zero address Feb 1, 2023
@cyberhorsey
Copy link
Contributor

I think this should actually be true for allowing the 0 address, as we only deploy an EtherVault on Layer 2, not Layer 1?

@davidtaikocha
Copy link
Member

I think this should actually be true for allowing the 0 address, as we only deploy an EtherVault on Layer 2, not Layer 1?

I think so.

@dionysuzx dionysuzx changed the title refactor(protocol): remove redundant check for zero address fix(protocol): allow zero address for ethervault Feb 2, 2023
@dionysuzx dionysuzx changed the title fix(protocol): allow zero address for ethervault fix(protocol): allow resolver to return zero address for EtherVault Feb 2, 2023
@dionysuzx
Copy link
Collaborator Author

@dantaik i applied the suggested changes, thanks. but i'm curious, why wasn't this reverting then? it should've been throwing errors when withdrawing on layer 1 right?

@davidtaikocha
Copy link
Member

davidtaikocha commented Feb 3, 2023 •
edited
Loading

@dantaik i applied the suggested changes, thanks. but i'm curious, why wasn't this reverting then? it should've been throwing errors when withdrawing on layer 1 right?

I believe its because the bool allowZeroAddress parameter of AddressReslover.reslove() function is introduced in a PR (signal service) which was merged after the Alpha-1 release. So this issue dose not exist in the testnet, and also since our bridge / relayer has not started integrating signal service yet, so they didn't find that issue neither?

@dionysuzx dionysuzx enabled auto-merge (squash) February 3, 2023 05:23
@dionysuzx dionysuzx requested a review from dantaik February 3, 2023 05:23
@dionysuzx dionysuzx merged commit cb34cf0 into main Feb 3, 2023
@dionysuzx dionysuzx deleted the remove-check branch February 3, 2023 07:17
@github-actions github-actions bot mentioned this pull request Feb 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidtaikocha davidtaikocha davidtaikocha approved these changes

@dantaik dantaik dantaik approved these changes

Assignees
No one assigned
Labels
area.protocol
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dionysuzx @cyberhorsey @davidtaikocha @dantaik