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

SNS Asset Canister page #1613

Merged
merged 17 commits into from
Jun 26, 2023
Merged

SNS Asset Canister page #1613

merged 17 commits into from
Jun 26, 2023

Conversation

jessiemongeon1
Copy link
Contributor

Thank you for your contribution to the IC Developer Portal.
Before submitting your Pull Request, please make sure that:

@jessiemongeon1 jessiemongeon1 requested a review from a team as a code owner June 22, 2023 20:56
@dprats
Copy link
Contributor

dprats commented Jun 22, 2023

Note the tests seem to be failing

Copy link
Contributor

@LaraAS LaraAS left a comment

Choose a reason for hiding this comment

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

Thank you very much for working on this.
I raised two high level questions / concerns on slack.
Very happy to have another look, but thought it might be easiest to clarify them first.

Copy link
Contributor

@LaraAS LaraAS left a comment

Choose a reason for hiding this comment

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

I am still not sure what version we write the doc for - the "old" version where a dev on a list can do the upgrade or the "new" version where the upgarde happens by proposal.
The intro seems to describe the former, while the details seem to show the latter.
Let me follow up with the SDK team on slack!

Copy link
Contributor

@LaraAS LaraAS left a comment

Choose a reason for hiding this comment

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

After learning that we want to describe the new flow, I made another pass.

I suggest 2 larger changes:

  1. I think it would be very helpful to introduce the different roles and maybe also how one can find them out when given an asset canister (I assume there is a command)
  2. It would be helpful to have a section about what needs to be done during an SNS launch to ensure that the proposal in the last section work (hand over cansiter, set roles correctly, register generic proposals)

WDYT? Happy to discuss further!

Copy link
Contributor

@LaraAS LaraAS left a comment

Choose a reason for hiding this comment

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

sending a first batch of comments - hope to send more after next meeting.

I think the new description of permissions is super helpful - thanks a lot for the changes!

Copy link
Contributor

@LaraAS LaraAS left a comment

Choose a reason for hiding this comment

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

Done with the second part. Please feel free to merge whenever you feel ready (as I don't want to add another day just due to the time zone).
I think it is looking pretty good now!

@jessiemongeon1 jessiemongeon1 merged commit d85fbed into master Jun 26, 2023
2 checks passed
@jessiemongeon1 jessiemongeon1 deleted the sns-asset-canister branch June 26, 2023 22:13
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.

4 participants