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.

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: superchain #84

Merged
merged 16 commits into from
Oct 16, 2024
Merged

feat: superchain #84

merged 16 commits into from
Oct 16, 2024

Conversation

pedrovalido
Copy link
Contributor

No description provided.

@pedrovalido pedrovalido requested review from stas and ethzoomer October 4, 2024 17:00
Copy link
Collaborator

@stas stas left a comment

Choose a reason for hiding this comment

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

Looks great if these are all the changes we need to make!!! Love it!

My only concern is the factory registry. I think we need one even though it's for Sugar needs.

I think the registry, which we can deploy just once, is a much better way to avoid human errors. We've had enough issues where we'd forget to update the env vars here and end up with a bad release. So just trying to limit such an outcome!

@@ -259,21 +259,23 @@ interface IAlmLpWrapper:

# Vars
registry: public(IFactoryRegistry)
voter: public(IVoter)
voter: public(IVoter) # Voter on root , LeafVoter on leaf chain
factories: public(DynArray[address, MAX_FACTORIES])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would like to reconsider this direction.

@@ -6,6 +6,7 @@

def main():
contract_name = str(os.getenv('CONTRACT')).lower()
chain_name = str(os.getenv('CHAIN')).upper()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would like to switch to chain IDs instead. We do the same for web app.
Chane names are inconsistent and error prone.

Consider the format VOTER_${chain_id}=0x...0001 etc..

@pedrovalido pedrovalido force-pushed the feat/superchain-sugar branch 3 times, most recently from 1784b81 to 3ce3cc2 Compare October 6, 2024 15:14
@pedrovalido pedrovalido force-pushed the feat/superchain-sugar branch from 3ce3cc2 to a4e9691 Compare October 7, 2024 16:05
(cherry picked from commit 1cf7a2d)
(cherry picked from commit f622917)
@stas stas changed the title feat: general sugar for all chains, integrate superchain feat: superchain Oct 15, 2024
Copy link
Collaborator

@stas stas left a comment

Choose a reason for hiding this comment

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

:shipit: 🔴

@pedrovalido pedrovalido merged commit d3f8699 into v3 Oct 16, 2024
2 checks passed
@pedrovalido pedrovalido deleted the feat/superchain-sugar branch October 16, 2024 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants