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-86drpncxu - Adicionar um comportamento novo no setMetaData #11

Merged
merged 1 commit into from
Apr 15, 2024

Conversation

luc10921
Copy link
Contributor

No description provided.

@luc10921 luc10921 requested review from melanke and raulduartep April 12, 2024 12:50
@luc10921 luc10921 self-assigned this Apr 12, 2024
@melanke
Copy link
Member

melanke commented Apr 12, 2024

@public(name="setMetaData")
def set_meta_data(script_hash: UInt160, property_name: str, value: str) -> bool:
@public(name="setMetadata")
def set_owner_and_metadata(owner: UInt160, contract_script_hash: UInt160, property_name: str, value: str) -> bool:

Choose a reason for hiding this comment

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

The function name set_owner_and_metadata is not very descriptive. Consider renaming it to something more specific, like set_contract_owner_and_metadata.

has_ownership = set_ownership(contract_script_hash, owner)

if not has_ownership:
raise Exception('No authorization')

Choose a reason for hiding this comment

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

The exception message 'No authorization' is not very descriptive. Consider changing it to 'Failed to set ownership: No authorization'.


return set_metadata(contract_script_hash, property_name, value)

@public(name="setMetadata")

Choose a reason for hiding this comment

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

The function name setMetadata is used twice. Consider renaming one of the functions to avoid confusion.

contractScriptHash,
contractOwner: user.account.scriptHash,
})
assert(userCanChangeOwnership)
})

it('Tests canChangeMetaData with verify method', async () => {
it('Tests canChangeMetadata with verify method', async () => {

Choose a reason for hiding this comment

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

Please ensure that the function canChangeMetadata exists and is correctly implemented, as the original function was canChangeMetaData.

const user = wallets.find((wallet: any) => wallet.name === 'user')
const iconDappUser = await getSdk(user.account)

const {stdout} = await exec(`neoxp contract get "verifiable" -i ${neoXpInstancePath}`)
const contractScriptHash = JSON.parse(stdout)[0].hash

// user is checking if they can change ownership of the smart contract without adding the contract as a signer
const userCanChangeOwnershipNoSigner = await iconDappUser.canChangeMetaData({
const userCanChangeOwnershipNoSigner = await iconDappUser.canChangeMetadata({
contractScriptHash,
contractOwner: user.account.scriptHash,
})
assert(userCanChangeOwnershipNoSigner === false )

Choose a reason for hiding this comment

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

Consider using assert.strictEqual(userCanChangeOwnershipNoSigner, false) for a strict equality check.

contractScriptHash,
contractOwner: user.account.scriptHash,
})
assert(userCanChangeOwnershipNoSigner === false )

// user is checking if they can change ownership of the smart contract
const userCanChangeOwnership = await iconDappUser.canChangeMetaData(
const userCanChangeOwnership = await iconDappUser.canChangeMetadata(
{
contractScriptHash,
contractOwner: user.account.scriptHash,

Choose a reason for hiding this comment

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

The method canChangeMetadata is being called with the same parameters twice. If the result is expected to be different, please ensure the state is correctly modified between the calls.

@@ -652,7 +761,7 @@ describe('Basic IconDapp Test Suite', function () {
assert.strictEqual(icons.some(icon => icon.scriptHash==="0x48c40d4666f93408be1bef038b6722404d9a4c2a"), true)
assert.strictEqual(icons.some(icon => icon.scriptHash==="0xf15976ea5c020aaa12b9989aa9880e990eb5dcc9"), true)

const txid = await iconDapp.setMetaData({
const txid = await iconDapp.setMetadata({

Choose a reason for hiding this comment

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

Please ensure that the method name change from setMetaData to setMetadata is reflected throughout the codebase to maintain consistency and avoid potential errors.

@@ -686,7 +795,7 @@ describe('Basic IconDapp Test Suite', function () {
assert.strictEqual(icons.some(icon => icon.scriptHash==="0x48c40d4666f93408be1bef038b6722404d9a4c2a"), true)
assert.strictEqual(icons.some(icon => icon.scriptHash==="0xf15976ea5c020aaa12b9989aa9880e990eb5dcc9"), true)

const txid = await iconDapp.setMetaData({
const txid = await iconDapp.setMetadata({

Choose a reason for hiding this comment

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

The method name has been changed from setMetaData to setMetadata. Please ensure that this change is reflected everywhere in the code where this method is called to avoid any potential errors.

@melanke melanke merged commit bdaea88 into main Apr 15, 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