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

new CW721_ADMIN store #81

Merged
merged 3 commits into from
Jan 23, 2024
Merged

Conversation

taitruong
Copy link
Collaborator

new store allows to provide e.g. Some(addr) or None which will be used for setting admin during instantiation of cw721.

@taitruong
Copy link
Collaborator Author

pls review @humanalgorithm @shanev @jhernandezb

@@ -506,6 +529,19 @@ where
.add_attribute(
"cw721_base_code_id",
cw721_base_code_id.map_or_else(|| "none".to_string(), |or| or.to_string()),
)
.add_attribute(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain what this line is doing?

Copy link
Collaborator Author

@taitruong taitruong Jan 22, 2024

Choose a reason for hiding this comment

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

Here cw721_base_code_id is of type Option<u64>. In response it just say "none" is provided - instead of returning an empty string.

Copy link
Contributor

Choose a reason for hiding this comment

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

My question here is why do we have the plaintext "none" instead of an actual None?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Attribute must be a string, that's why we need to convert all these data.

@@ -38,6 +38,8 @@ pub const INCOMING_CLASS_TOKEN_TO_CHANNEL: Map<(ClassId, TokenId), String> = Map
/// is `None`) is stored in this map. When the token is returned to
/// it's source chain, the metadata is removed from the map.
pub const TOKEN_METADATA: Map<(ClassId, TokenId), Option<Binary>> = Map::new("j");
/// The admin address for instantiating new cw721 contracts. In case of None, contract is immutable.
pub const CW721_ADMIN: Item<Option<Addr>> = Item::new("l");
Copy link
Contributor

Choose a reason for hiding this comment

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

Main point of the PR is here

Copy link
Contributor

Choose a reason for hiding this comment

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

Per discussion, this name doesn't really describe what's going on. Please rename to something like ADMIN_USED_FOR_CW721.

The point of this store is to *copy the admin here when instantiating new cw721 contracts

@humanalgorithm humanalgorithm requested a review from tasiov January 22, 2024 19:05
@taitruong taitruong requested review from jhernandezb and tasiov and removed request for jhernandezb and tasiov January 22, 2024 21:07
@taitruong taitruong added this pull request to the merge queue Jan 23, 2024
Merged via the queue into public-awesome:main with commit 09da354 Jan 23, 2024
1 check passed
@taitruong taitruong deleted the cw721_admin branch January 23, 2024 09: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.

4 participants