-
Notifications
You must be signed in to change notification settings - Fork 0
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
Refactor x/collection,token proto #3
Conversation
// base img uri is an uri for the contract image stored off chain. | ||
string base_img_uri = 3; | ||
// uri for the resource of the contract stored off chain. | ||
string uri = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the following comment also needs to be modified.
// - `base_img_uri` exceeds the app-specific limit in length.
https://github.com/line/lbm-sdk/blob/v0.46.0/proto/lbm/collection/v1/tx.proto#L360
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment would be removed in the subsequent PR, in favor of codespaced errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please refer: Finschia#849
proto/lbm/collection/v1/tx.proto
Outdated
@@ -603,14 +603,14 @@ message MsgModify { | |||
// changes to apply. | |||
// on modifying collection: name, base_img_uri, meta. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// on modifying collection: name, base_img_uri, meta. | |
// on modifying collection: name, uri, meta. |
It's better to unify the terms into uri
.
@@ -74,10 +74,10 @@ enum AttributeKey { | |||
ATTRIBUTE_KEY_NEW_ROOT_TOKEN_ID = 19 [(gogoproto.enumvalue_customname) = "AttributeKeyNewRoot"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does enum AttributeKey
affect legacy events?
If not, I guess the following definition should be modified into ATTRIBUTE_KEY_URI
and AttributeKeyURI
. (Possibly AttributeKeyUri
is better?)
ATTRIBUTE_KEY_BASE_IMG_URI = 8 [(gogoproto.enumvalue_customname) = "AttributeKeyBaseImgURI"];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does. So we would better not touch it.
// image_uri is an uri for the image of the token class stored off chain. | ||
string image_uri = 3; | ||
// uri for the resource of the token class stored off chain. | ||
string uri = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the following comment also needs to be modified.
// - `image_uri` exceeds the app-specific limit in length.
https://github.com/line/lbm-sdk/blob/v0.46.0/proto/lbm/token/v1/tx.proto#L239
@@ -51,10 +51,10 @@ enum AttributeKey { | |||
ATTRIBUTE_KEY_PROXY = 14 [(gogoproto.enumvalue_customname) = "AttributeKeyProxy"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does enum AttributeKey
affect legacy events?
If not, I guess the following definition should be modified into ATTRIBUTE_KEY_URI
and AttributeKeyURI
. (Possibly AttributeKeyUri
is better?)
ATTRIBUTE_KEY_IMG_URI = 8 [(gogoproto.enumvalue_customname) = "AttributeKeyImageURI"];
// contract id associated with the contract. | ||
string contract_id = 1; | ||
// the address of the grantee which must have modify permission. | ||
string owner = 2; | ||
string operator = 2; | ||
// changes to apply. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain possible attribute keys in the comment, so that client developers accurately know them?
For example,
// changes to apply.
// Possible keys: uri, meta, .........
repeated Attribute changes = 3 [(gogoproto.nullable) = false];
Note that the comment of message MsgModify
is already providing such information.
https://github.com/line/lbm-sdk/blob/v0.46.0/proto/lbm/collection/v1/tx.proto#L603-L605
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the information.
@@ -248,10 +248,10 @@ message EventModifiedContract { | |||
repeated Attribute changes = 3 [(gogoproto.nullable) = false]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about explaining possible keys of changes
like the following?
// changes of the attributes applied.
// The possible attribute keys are same as the possible keys of `changes` in `message MsgModify`.
repeated Attribute changes = 3 [(gogoproto.nullable) = false];
@@ -264,10 +264,10 @@ message EventModifiedTokenClass { | |||
string type_name = 5; | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, how about explaining possible keys of changes
like the following?
// changes of the attributes applied.
// The possible attribute keys are same as the possible keys of `changes` in `message MsgModify`.
repeated Attribute changes = 4 [(gogoproto.nullable) = false];
@@ -278,10 +278,10 @@ message EventModifiedNFT { | |||
repeated Attribute changes = 4 [(gogoproto.nullable) = false]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, how about explaining possible keys of changes
like the following?
// changes of the attributes applied.
// The possible attribute keys are same as the possible keys of `changes` in `message MsgModify`.
repeated Attribute changes = 4 [(gogoproto.nullable) = false];
proto/lbm/token/v1/event.proto
Outdated
// | ||
// Since: 0.46.0 (finschia) | ||
message EventModified { | ||
message EventModifyContract { | ||
// contract id associated with the token class. | ||
string contract_id = 1; | ||
// address which triggered the modify. | ||
string operator = 2; | ||
// changes on the metadata of the class. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, how about explaining possible keys of changes
like the following?
// changes on the metadata of the class.
// The possible attribute keys are same as the possible keys of `changes` in `message MsgModifyContract`.
repeated Attribute changes = 3 [(gogoproto.nullable) = false];
It seems that the term "proxy" is still being used in the following tx messages.
Shall we rename them to "operator" if we make the decision to adopt the term "operator"? |
Description
Changes applied (the list is not exhaustive):
Attribute
forModify
and the corresponding events.uri
contract_id
->id
(where it's obvious in the context)id
->contract_id
,class_id
(where it's ambiguous)from
->operator
(on mint)owner
->operator
(on burn)from
->grantee
orgranter
(on grant & renounce)TokenClass
intoContract
because the contents of its id is a contract id (x/token).Operator
variants overFrom
variantsSend
variants overTransfer
variantsAuthorizeOperator
variants overApprove
variantsMotivation and context
How has this been tested?
Screenshots (if appropriate):
Checklist:
CHANGELOG.md
client/docs/swagger-ui/swagger.yaml