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/add upgradabeality to eoa #439

Merged
merged 10 commits into from
Oct 25, 2023

Conversation

bajpai244
Copy link
Collaborator

Pull Request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

This PR adds upgradeability to the EOA contract.

Resolves: #433

What is the new behavior?

The EOA is now an upgradeable contract.

Does this introduce a breaking change?

  • Yes
  • No

@enitrat
Copy link
Collaborator

enitrat commented Oct 23, 2023

@Eikix can you review

Copy link
Member

@Eikix Eikix left a comment

Choose a reason for hiding this comment

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

lgtm!
can we add some TODOs:

  • create a way to make EOA.upgrade reachable (currently EOA flow cannot call upgrade)
  • create a way to make EOA.upgrade opinionated on target implementations it can upgrade to (some security on interfaces) -> low prio

@bajpai244 bajpai244 force-pushed the feat/add-upgradabeality-to-eoa branch from 30753e6 to 82ffc83 Compare October 23, 2023 11:04
@bajpai244
Copy link
Collaborator Author

lgtm! can we add some TODOs:

  • create a way to make EOA.upgrade reachable (currently EOA flow cannot call upgrade)
  • create a way to make EOA.upgrade opinionated on target implementations it can upgrade to (some security on interfaces) -> low prio

@Eikix have added the TODO via this commit.

Copy link
Member

@Eikix Eikix left a comment

Choose a reason for hiding this comment

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

one question

crates/contracts/src/tests/test_kakarot_core.cairo Outdated Show resolved Hide resolved
Copy link
Member

@Eikix Eikix left a comment

Choose a reason for hiding this comment

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

lgtm, will let @enitrat have a quick look

Eikix
Eikix previously approved these changes Oct 24, 2023
Copy link
Member

@Eikix Eikix left a comment

Choose a reason for hiding this comment

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

lgtm

@Eikix
Copy link
Member

Eikix commented Oct 24, 2023

Nvm approved but you were cursed with the scarb fmt --check fail;D
This happens all too often, should we create a pre-push commit hook?:))

Eikix
Eikix previously approved these changes Oct 24, 2023
Copy link
Member

@Eikix Eikix left a comment

Choose a reason for hiding this comment

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

lgtm

enitrat
enitrat previously approved these changes Oct 24, 2023
@Eikix
Copy link
Member

Eikix commented Oct 24, 2023

Nvm, failing CI:)

@enitrat
Copy link
Collaborator

enitrat commented Oct 25, 2023

@bajpai244 you need to re-compute the expected starknet address, as the class hash has changed

@Eikix
Copy link
Member

Eikix commented Oct 25, 2023

@bajpai244 you need to re-compute the expected starknet address, as the class hash has changed

Which class hash?

@enitrat
Copy link
Collaborator

enitrat commented Oct 25, 2023

@bajpai244 you need to re-compute the expected starknet address, as the class hash has changed

Which class hash?

EOA class hash

@Eikix Eikix dismissed stale reviews from enitrat and themself via 50fee63 October 25, 2023 08:53
@Eikix Eikix force-pushed the feat/add-upgradabeality-to-eoa branch from 26b6400 to 50fee63 Compare October 25, 2023 08:53
Eikix
Eikix previously approved these changes Oct 25, 2023
Copy link
Member

@Eikix Eikix left a comment

Choose a reason for hiding this comment

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

lord stop trolling me pls

@Eikix Eikix merged commit b104729 into kkrt-labs:main Oct 25, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: make EOA upgradable
3 participants