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

Extract colony metadata from colony model #296

Merged
merged 12 commits into from
Mar 9, 2023

Conversation

jakubcolony
Copy link
Collaborator

@jakubcolony jakubcolony commented Feb 28, 2023

Description

This PR extracts the colony metadata (the information that is not available on the chain) into its own model, allowing the client to write it separately from the "core" colony details.

To achieve this, I removed the Colony<>Profile relation. The display name, avatar, and thumbnail are now properties of Colony Metadata. This is to reduce complexity (you don't need either an extra mutation to create profile, or a custom lambda function like createUniqueColony) and allow for easier tracking of the changes (similarly to domain metadata's changelog).

I don't think it is yet possible to move the entire colony creation mutation to block-ingestor and only write the metadata in the saga (as we do with the domain). The problem I faced is that colony name isn't available on chain but at the same time it has to be unique and indexable (since we want to get colony by name reliably and fast). This needs further thought but one idea could be to create some mapping between given names and colony addresses (as well as chains possibly).

Testing

Testing mostly focuses on double-checking I haven't broken anything. Please create some colonies using both the creation wizard and temp-create-data script. Inspect the CDapp to make sure none of the previous profile data (just display name for now) isn't missing/wrong.

Resolves #292

@jakubcolony jakubcolony self-assigned this Feb 28, 2023
@jakubcolony jakubcolony marked this pull request as ready for review March 6, 2023 23:10
@@ -27,7 +27,7 @@ input ProfileMetadataInput {
emailPermissions: [String!]!
}

input MetadataInput {
input ChainMetadataInput {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed to avoid confusion between all the different metadatas we now have

@jakubcolony jakubcolony requested a review from a team March 6, 2023 23:11
Base automatically changed from port/282-edit-domain to port/colony-actions March 7, 2023 14:28
Copy link
Contributor

@Julianahlk Julianahlk left a comment

Choose a reason for hiding this comment

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

Tested everything I could think of and couldn't find any issues. I do notice there are some merge conflicts. I can take another look once those are resolved.

@rdig
Copy link
Member

rdig commented Mar 8, 2023

Does this need to be rebased ? as it shows 70 changed files. I don't think that correct ?

@jakubcolony
Copy link
Collaborator Author

@rdig Done

Copy link
Member

@rdig rdig left a comment

Choose a reason for hiding this comment

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

Yup, sensible change.

No complaints from me. I just need to keep an eye out for when I merge #74 to make sure I update it to make use of the new naming scheme

@@ -253,7 +257,25 @@ const createMetacolony = async (singerOrWallet) => {
if (metacolonyQuery?.errors) {
console.log('METACOLONY COULD NOT BE CREATED.', metacolonyQuery.errors[0].message);
} else {
console.log(`Creating metacolony { name: "meta", colonyAddress: "${utils.getAddress(metacolonyAddress)}", profile: { displayName: "Metacolony" }, nativeToken: "${utils.getAddress(metacolonyTokenAddress)}", version: "${metacolonyVersion.toString()}" }`);
console.log(`Creating metacolony { name: "meta", colonyAddress: "${utils.getAddress(metacolonyAddress)}", nativeToken: "${utils.getAddress(metacolonyTokenAddress)}", version: "${metacolonyVersion.toString()}" }`);
Copy link
Member

Choose a reason for hiding this comment

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

Is the temp script necessary anymore ?

We should decide what to do with it. Either:

  • remove it and create everything manually
  • keep it, but update it so that it creates proper colonies (with extensions, token authorities and everything)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, maintaining it is a bit of a pain. But it's nice to have the script that bootstraps everything for you.
One option could be to use some headless browser (cypress, puppeteer) to programmatically click through the user and then colony creation wizard, but I don't know how feasible that would be

Copy link
Member

@rdig rdig Mar 8, 2023

Choose a reason for hiding this comment

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

We had that before, and it more or less worked, however it provided quite a bit of overhead and ate alot of resources.

I guess once we stabilize the core features, maintaining this won't be as much of a problem anymore

@jakubcolony jakubcolony merged commit 86ae382 into port/colony-actions Mar 9, 2023
@jakubcolony jakubcolony deleted the refactor/292-colony-metadata branch March 9, 2023 17:34
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.

3 participants