-
Notifications
You must be signed in to change notification settings - Fork 0
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
SOV-4316 upgradeable contract #3
base: bobDevelopment
Are you sure you want to change the base?
Conversation
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.
- https://github.com/DistributedCollective/sovryn-lb-dex/blob/SOV-4316-upgradeable-contract/src/LBPair.sol#L80 We don’t need this variable in the LBPair logic contract - it comes from the Clone contract but we don’t use Clone contract, so pls remove it from the import too
- According to the ticket #SOV-4316 the contracts should be upgradable using OZ transparent upgradeable pattern, pls refer to: https://docs.openzeppelin.com/upgrades-plugins/1.x/foundry-upgrades#examples
- also see inline code comments
2/ address comment review
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.
another round of review completed.
after addressing new comments, we should be good to go.
apart from the code comments:
- the compiler is complaining on the unused param
Version
in the function_getLBPairInformation
- pls remove it. - according to the foundry docs
vm.createSelectFork
will not work for deployment on live chains but on a fork
if using --chain as a param then no need to change network - it will be used in the script it under the hood.
block.chainid can be used in the script to identify which deployment config is required.
if you want to switch networks the right way seems to set the chainId viavm.chainId(60808)
.
update:
it will work, but still confusing and not clear why setting a fork does deployment on a live chain, could be an issue with foundry, so usingvm.chainid
is more appropriate, but leaving it to you to decide on that.
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.
Overall good stuff!
Some changes requested, mostly about git related stuff (there's a ton of unneeded crap in the abi folder which made the review pretty cumbersome too). I also added one comment about removing create2
in LBFactory
, and some minor ones here and there.
I didn't look into the deploy scripts or tests too carefully.
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.
just to push pending comments
…-contract-alt Fix EIP-170 LBPair size limit
No description provided.