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

CU-86dt24n2g - Allow user to add/change icon link if they can verify … #10

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

luc10921
Copy link
Contributor

@luc10921 luc10921 commented Apr 2, 2024

…themselves on the smart contract

@luc10921 luc10921 requested a review from melanke April 2, 2024 19:54
@luc10921 luc10921 self-assigned this Apr 2, 2024
@melanke
Copy link
Member

melanke commented Apr 2, 2024

def get_deployer(script_hash: UInt160, sender: UInt160) -> Optional[UInt160]:
contract: Contract = ContractManagement.get_contract(script_hash)
@public(name='canChangeMetaData', safe=True)
def can_change_meta_data(contract_script_hash: UInt160, contract_owner: UInt160) -> bool:
Copy link

Choose a reason for hiding this comment

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

Consider renaming the function can_change_meta_data to can_change_metadata to follow the camelCase naming convention.

Copy link
Member

@melanke melanke Apr 8, 2024

Choose a reason for hiding this comment

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

Metadata is a single word. We should change on the annotation as well to canChangeMetadata

contract/icondapp.py Show resolved Hide resolved
contract/icondapp.py Show resolved Hide resolved
has_verify = True
break
if has_verify:
if runtime.check_witness(contract_script_hash):
Copy link

Choose a reason for hiding this comment

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

Consider handling the case where runtime.check_witness(contract_script_hash) returns None. This could potentially lead to a NoneType error.

Copy link
Member

@melanke melanke Apr 8, 2024

Choose a reason for hiding this comment

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

Is it possible for the check_witness to return None?


current_contract_owner: UInt160 = get_contract_owner(contract_script_hash)
if isinstance(current_contract_owner, UInt160):
if runtime.check_witness(current_contract_owner):
Copy link

Choose a reason for hiding this comment

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

Consider handling the case where runtime.check_witness(current_contract_owner) returns None. This could potentially lead to a NoneType error.

Copy link
Member

@melanke melanke Apr 8, 2024

Choose a reason for hiding this comment

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

Again, is it possible for the check_witness to return None?

def get_deployer(script_hash: UInt160, sender: UInt160) -> Optional[UInt160]:
contract: Contract = ContractManagement.get_contract(script_hash)
@public(name='canChangeMetaData', safe=True)
def can_change_meta_data(contract_script_hash: UInt160, contract_owner: UInt160) -> bool:
Copy link
Member

@melanke melanke Apr 8, 2024

Choose a reason for hiding this comment

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

Metadata is a single word. We should change on the annotation as well to canChangeMetadata

has_verify = True
break
if has_verify:
if runtime.check_witness(contract_script_hash):
Copy link
Member

@melanke melanke Apr 8, 2024

Choose a reason for hiding this comment

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

Is it possible for the check_witness to return None?


current_contract_owner: UInt160 = get_contract_owner(contract_script_hash)
if isinstance(current_contract_owner, UInt160):
if runtime.check_witness(current_contract_owner):
Copy link
Member

@melanke melanke Apr 8, 2024

Choose a reason for hiding this comment

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

Again, is it possible for the check_witness to return None?


return sender
# check if the contract was deployed by the given address
computed_script_hash: bytes = _compute_contract_hash(contract_owner, contract)
Copy link
Member

@melanke melanke Apr 8, 2024

Choose a reason for hiding this comment

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

Is it possible for the _compute_contract_hash to return None?

@melanke melanke merged commit acdbae5 into main Apr 8, 2024
1 check passed
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.

2 participants