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: added migration & ownable #166

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: added migration & ownable #166

wants to merge 4 commits into from

Conversation

leohhhn
Copy link
Contributor

@leohhhn leohhhn commented Sep 26, 2023

Description

This PR introduces ownership & realm migration functionality to the chess package, and addresses issue #56.

This PR also introduces an init function to the Chess realm, setting the realm owner to its deployer keypair upon deployment. The following functionality is added:

Ownership functionality:

  • owner variable is added to realm state,
  • ownership can be transferred to different address by current owner,
  • calling isOwner() will check if the caller is owner and will panic if the caller is not the owner,

Migration functionality:

  • migrateTo variable added to represent the path of new deployment of realm
  • migrated bool added to indicate if a migration has happened before
  • Migrate function to allow migrating, only callable by current owner of realm
  • All exported functions are uncallable if realm has migrated - check done by checkMigrated

@leohhhn leohhhn self-assigned this Sep 26, 2023
@netlify
Copy link

netlify bot commented Sep 26, 2023

Deploy Preview for gnochess-signup-form canceled.

Name Link
🔨 Latest commit acb5574
🔍 Latest deploy log https://app.netlify.com/sites/gnochess-signup-form/deploys/6512e52ccba57c000859e440

@netlify
Copy link

netlify bot commented Sep 26, 2023

Deploy Preview for gnochess canceled.

Name Link
🔨 Latest commit acb5574
🔍 Latest deploy log https://app.netlify.com/sites/gnochess/deploys/6512e52c52d3e300086f31ae

Copy link
Collaborator

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Left a few minor comments

Please add some unit tests for the migration functionality 🙏

package chess

func checkMigrated() {
if migrated {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if !migrated {return}
...

Copy link
Contributor Author

@leohhhn leohhhn Sep 26, 2023

Choose a reason for hiding this comment

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

Do you intend to handle the situation where the realm has not migrated yet? If that is the case, since migrated is a bool, the only stateful change that will happen is if it is true. Why check otherwise?

}

func Migrate(newRealmPath string) string {
// TODO add check for path validity
Copy link
Collaborator

Choose a reason for hiding this comment

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

Leftover TODO?

Copy link
Contributor Author

@leohhhn leohhhn Sep 26, 2023

Choose a reason for hiding this comment

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

Not sure how to check for path validity at this point. The check should be able to see whether or not there is a ream deployed at newRealmPath.

Edit: added an empty string check for now.

return true
}

func GetOwner() std.Address {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this exported?

Copy link
Contributor Author

@leohhhn leohhhn Sep 26, 2023

Choose a reason for hiding this comment

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

So that it can be called by gnokey, as to check who is the owner of realm

return migratedTo
}

// testing commands, todo remove
Copy link
Collaborator

Choose a reason for hiding this comment

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

Leftover?

Copy link
Contributor Author

@leohhhn leohhhn Sep 26, 2023

Choose a reason for hiding this comment

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

Left it in so its easier for anyone to test currently, will remove before merging
Removed.

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