Skip to content

Conversation

@deniszagumennov
Copy link
Contributor

@deniszagumennov deniszagumennov commented Feb 12, 2025

Based on #172 Pull Request, another attempt to update CosmWasm in the contracts

@deniszagumennov deniszagumennov force-pushed the main branch 2 times, most recently from 6cd821c to 45c8e5e Compare February 13, 2025 16:42
@deniszagumennov deniszagumennov force-pushed the main branch 10 times, most recently from 0822058 to 8e32724 Compare February 14, 2025 12:53
@deniszagumennov deniszagumennov marked this pull request as ready for review February 14, 2025 14:04
{
// if Some(bool), bool represents the `is_owner` check result.
// if None, `info` was none and the check is skipped.
Ok(is_minter) => is_minter.unwrap_or(true),
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this variable should be named something like is_minter_owner ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed it

let creator_initialized = CREATOR.item.may_load(deps.storage)?;
let is_creator_or_uninitialized =
match sender.map(|addr| is_owner(deps.storage, addr)).transpose() {
// if Some(bool), bool represents the `is_owner` check result.
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally I'm wary of comments explaining code, is there a way to name the variables so its self documenting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried, please check it out

Copy link
Member

@shanev shanev left a comment

Choose a reason for hiding this comment

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

LGTM as long as the comments by @jhernandezb and @humanalgorithm are addressed.

@deniszagumennov deniszagumennov force-pushed the main branch 3 times, most recently from fbe6463 to ea22eb1 Compare February 26, 2025 17:21
Copy link
Member

@shanev shanev left a comment

Choose a reason for hiding this comment

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

LGTM

@shanev shanev merged commit 4377867 into public-awesome:main Feb 26, 2025
7 checks passed
@shanev
Copy link
Member

shanev commented Feb 26, 2025

Thank you @deniszagumennov !!

@jhernandezb jhernandezb mentioned this pull request Feb 26, 2025
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.

6 participants