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: [N-01] Constants not using UPPER_CASE format #322

Merged
merged 2 commits into from Jul 28, 2023
Merged

fix: [N-01] Constants not using UPPER_CASE format #322

merged 2 commits into from Jul 28, 2023

Conversation

pxrl
Copy link
Contributor

@pxrl pxrl commented Jul 25, 2023

No description provided.

@pxrl pxrl requested a review from nicholaspai July 25, 2023 13:38
@pxrl
Copy link
Contributor Author

pxrl commented Jul 25, 2023

Note: I didn't touch the zkSync and zkSyncErc20Bridge variables. They're marked as constants, but since they're actually interfaces that have been instantiated, they feel somehow less constant-y. Not sure whether my intuition on that is right or not, so I wanted to flag it.

pxrl
pxrl commented Jul 25, 2023
View reviewed changes
contracts/chain-adapters/ZkSync_Adapter.sol Outdated
// gets aliased.
address public constant l2RefundAddress = 0x428AB2BA90Eba0a4Be7aF34C9Ac451ab061AC010;
address public constant L2_REFUND_ADDRESS = 0x428AB2BA90Eba0a4Be7aF34C9Ac451ab061AC010;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nb. The change here is also related to N-03, so it may need to change again.

@pxrl pxrl force-pushed the pxrl/n01-contants branch 2 times, most recently from 3a5ad70 to 22c16df Compare July 25, 2023 14:38
@pxrl pxrl requested a review from mrice32 July 25, 2023 14:52
mrice32
mrice32 approved these changes Jul 26, 2023
View reviewed changes
Copy link
Contributor

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks great! I would probably just change the interfaces, too, as they suggest, even if it does feel a bit weird. The linter also picks up on these, so I think the suggestion is technically correct.

@mrice32
remove refund address change
ce6e2e1 
Signed-off-by: Matt Rice <matthewcrice32@gmail.com>
@mrice32
Copy link
Contributor

mrice32 commented Jul 28, 2023

Everything looks great! I would probably just change the interfaces, too, as they suggest, even if it does feel a bit weird. The linter also picks up on these, so I think the suggestion is technically correct.

@pxrl I checked the other contracts, and I agree the way you did this was consistent. Due to the change in #326, I dropped the refund address change. So I think this should be ready to go.

@mrice32 mrice32 merged commit bf98ded into master Jul 28, 2023
5 checks passed
@mrice32 mrice32 deleted the pxrl/n01-contants branch July 28, 2023 01:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrice32 mrice32 mrice32 approved these changes

@nicholaspai nicholaspai Awaiting requested review from nicholaspai

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@pxrl @mrice32