-
Notifications
You must be signed in to change notification settings - Fork 4
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
better metadata reading to tokens from being stuck in the bridge #249
Conversation
/// encoding the function signature of N | ||
abi.encodeWithSignature("name()") | ||
); | ||
if (!successNameCall) { |
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 a reason for our fallbacks to be named subtly differently from the way upstream does it?
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.
Code looks fine, except the comment that polygon upstream seems to be subtly different, it would be good to align it if there isn't a reason not to.
Also I would suggest either adding tests or making an issue or a TODO comment (either in the original code or in the place where the tests should go) that we should add tests.
I reused the functions from the bridge contract. It's kinda ucly code duplications, but I can not think of a better method, since I dont' wanna modify the zkevm-contracts repo and refactor the changes there, to keep the diff minimal to the polygon zkevm |
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.
Awesome, looks great.
I wouldn't worry about the fact that this copy-pastes some code, I think it's better to copy-paste than do something ugly to make a dependency work.
closes #222