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

Safe upgrades: Add healthcheck function and call it when upgrading a contract #116 -Malachi PR #122

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

scobi7
Copy link
Collaborator

@scobi7 scobi7 commented Aug 6, 2024

No description provided.

@scobi7 scobi7 requested a review from tensojka August 6, 2024 10:21
@scobi7
Copy link
Collaborator Author

scobi7 commented Aug 7, 2024

Fixed health check tests. Everything works fine now.

Copy link
Contributor

@tensojka tensojka left a comment

Choose a reason for hiding this comment

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

Why does get_latest_proposal_id exist? I think a remnant of past refactorings, should be removed now.

I think you're missing the actual healthcheck (before upgrade).

scobi7 added a commit to scobi7/konoha that referenced this pull request Aug 21, 2024
src/health.cairo Outdated
Comment on lines 75 to 100
fn assert_correct_contract_type(
self: @ContractState, proposed_contract_type: felt252, previous_contract_type: u64
) -> bool {
// Check if the proposed contract type is compatible with the previous contract type
if proposed_contract_type == 1 && previous_contract_type == 1 {
// Generic contract upgrade is allowed
true
} else if proposed_contract_type == 3 && previous_contract_type == 3 {
// Airdrop upgrade is allowed
true
} else if proposed_contract_type == 5 && previous_contract_type == 5 {
// Custom proposal is allowed
true
} else if proposed_contract_type == 6 && previous_contract_type == 6 {
// Arbitrary proposal is allowed
true
} else if (proposed_contract_type == 0 && previous_contract_type == 0)
|| (proposed_contract_type == 2 && previous_contract_type == 2) {
false
} else {
false
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect a function with assert in its name to panic if it is not the correct contract type. Either name is is_correct_contract_type and return bool or keep it like this but make it panic if it's not the correct type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I hope this isnt a stupid question-

I know I have to remove the get_latest_prop_id but I'm now confused. Is the way I'm approaching the health check so far the way you were expecting? Or is it completely wrong since I only need to run this for generic proposals. And in this case, what are the checks to ensure that a generic proposal is compatible with the governance state? (would it be things like payload, to_upgrade, or simply that it has a valid quorum, vote time etc.)

Im now confused on what the healthcheck is...

@scobi7 scobi7 force-pushed the scobi-safeUpgrades branch from 1d81911 to ab27fe4 Compare August 22, 2024 23:01
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.

2 participants