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

Update Credit Class Designer to "Credit Class Admin" #496

Closed
clevinson opened this issue Aug 25, 2021 · 10 comments · Fixed by #500
Closed

Update Credit Class Designer to "Credit Class Admin" #496

clevinson opened this issue Aug 25, 2021 · 10 comments · Fixed by #500

Comments

@clevinson
Copy link
Member

We had a call with the science team today where we deemed that Credit Class Designer is probably not the right name for what we mean (in x/ecocredit documentation & code).

It's often the case that someone will be "designing" a credit class, but not actually wanting to manage the process of managing a credit class.

Similarly, the AllowList should probably be called an allow list for "Credit Class Creators", as that's the technical capabilities this will have.

@ruhatch
Copy link
Contributor

ruhatch commented Aug 25, 2021

Is Credit Class Creator different than Credit Class Admin? The allowlist would be filtering the values of the Credit Class Admin field of the ClassInfo, right?

@ruhatch
Copy link
Contributor

ruhatch commented Aug 25, 2021

Also, this is in progress, but no one is assigned. Is there someone working on it?

@ryanchristo
Copy link
Member

ryanchristo commented Aug 26, 2021

Is Credit Class Creator different than Credit Class Admin? The allowlist would be filtering the values of the Credit Class Admin field of the ClassInfo, right?

The allowlist would be for credit class creators (someone who can create a credit class) and then the creator of the credit class would become the admin of the credit class. Using allowed credit class creators will help clarify what action can be taken by the addresses listed in the allowlist. One becomes an admin only after the credit class is created.

@ryanchristo
Copy link
Member

Also, this is in progress, but no one is assigned. Is there someone working on it?

I don't think anyone is working on it at the moment. Not sure why it was placed within in progress.

@ruhatch ruhatch self-assigned this Aug 26, 2021
@ruhatch
Copy link
Contributor

ruhatch commented Aug 26, 2021

Is Credit Class Creator different than Credit Class Admin? The allowlist would be filtering the values of the Credit Class Admin field of the ClassInfo, right?

The allowlist would be for credit class creators (someone who can create a credit class) and then the creator of the credit class would become the admin of the credit class. Using allowed credit class creators will help clarify what action can be taken by the addresses listed in the allowlist. One becomes an admin only after the credit class is created.

Cool. I've also just looked through the code and the creator and admin could be different. From the CLI, you'd need signatures from both the creator, specified with the --from flag, and the admin specified as a parameter.

I think we may need to update our logic in the CreateClass function to check the value in the --from flag, as they're the account doing the actual creation! @clevinson do you think that's an accurate reflection of the purpose of the allowlist? To clarify, we'd be check the actual creator against the allowlist, but then we could use any address in the admin field. I think this provides the safety that we want from the permissioning.

@ruhatch
Copy link
Contributor

ruhatch commented Aug 26, 2021

This would then look like this perhaps:

message MsgCreateClass {

  // creator is the address of the account which is creating the class. The
  // creator will be charged the fee to create a new class. The creator does not
  // have administrative permissions over the class unless they're also the
  // admin of the class.
  string creator = 1;

  // admin is the address of the account which can manage the credit class. The
  // admin has permissions to change the list of issuers and perform other
  // administrative operations.
  string admin = 2;

  ...
}

and the CreateClass message would have to be signed by both the Creator and the Admin. Is this what we want, or should we just constrain to Creator == Admin? @clevinson @ryanchristo @aaronc

@ryanchristo
Copy link
Member

ryanchristo commented Aug 26, 2021

I think we may need to update our logic in the CreateClass function to check the value in the --from flag, as they're the account doing the actual creation!

Yup, it looks like the logic needs to be updated. I think it makes sense to have the sender represent the creator (--from address) and to check the --from address rather than the admin address when checking the allowlist.

To clarify, we'd be check the actual creator against the allowlist, but then we could use any address in the admin field.

Yes, the credit class creator should be able to assign an admin and we should check the creator against the allowlist.

the CreateClass message would have to be signed by both the Creator and the Admin

Would it make more sense to have the creator pay the fee? We would only need a signature from the creator if so. The admin would just be an assignee in that case, so no signature required.

@clevinson
Copy link
Member Author

So I think @ryanchristo what you crossed out here is exactly what I understand the desired behavior to be.

We currently don't have a known use case for separate credit class creators & admins (making these distinct users).

IMO the desired behavior should be:

  • keep CLI & proto definitions as is (in terms of the number of parameters/fields), but rename CreditClassDesigner to CreditClassAdmin
  • in the allowlist, change it from being called a
  • check that a credit class admin is in the AllowList for credit class creators

@ruhatch if you think this terminology is still confusing, then maybe we should just call it all CreditClassAdmin and AdminAllowList? I know Ryan was originally expressing hesitation with that idea, but after reading how you interpreted the distinction btw creator & admin I'm wondering if we should reconsider.

@ruhatch
Copy link
Contributor

ruhatch commented Aug 27, 2021

So I think @ryanchristo what you crossed out here is exactly what I understand the desired behavior to be.

We currently don't have a known use case for separate credit class creators & admins (making these distinct users).

IMO the desired behavior should be:

  • keep CLI & proto definitions as is (in terms of the number of parameters/fields), but rename CreditClassDesigner to CreditClassAdmin
  • in the allowlist, change it from being called a
  • check that a credit class admin is in the AllowList for credit class creators

@ruhatch if you think this terminology is still confusing, then maybe we should just call it all CreditClassAdmin and AdminAllowList? I know Ryan was originally expressing hesitation with that idea, but after reading how you interpreted the distinction btw creator & admin I'm wondering if we should reconsider.

I think we could keep both terms, but we'd actually want to remove the CLI parameter for admin and instead take the admin from the --from flag, so that we don't need to check that the --from address is the same as the provided admin address. Then the rest of the logic stays the same. I'll work that up now.

@ryanchristo
Copy link
Member

Ok, I think it makes sense for now to have the creator become the admin.

I was hoping that we could support the assignment of an admin, in which case the creator could be on the approved list and the creator could assign an admin who is not on the approved list, but I realize this would require changes to the proto definitions and the permissioned approach is only meant to be temporary.

I think removing the CLI parameter for admin and using --from helps make things more clear from a user perspective. The admin parameter implies an admin can be assigned and that the creator (sender) and admin are different, which is not the case and which might cause confusion.

ruhatch added a commit that referenced this issue Sep 1, 2021
* refactor: Rename credit class designer to admin (#496)

* Take credit class admin from --from flag

* Add details to create-class CLI long description

* Fix failing tests

* Allow key-name in create-class --from flag

* Run make gen-proto
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants