Skip to content

feat: added migration & ownable#166

Open
leohhhn wants to merge 4 commits intomainfrom
feat/migrate
Open

feat: added migration & ownable#166
leohhhn wants to merge 4 commits intomainfrom
feat/migrate

Conversation

@leohhhn
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Leftover?

Copy link
Copy Markdown
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