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

fix: Hash computation over older item #454

Merged
merged 2 commits into from
Mar 21, 2022
Merged

Conversation

LautaroPetaccio
Copy link
Contributor

@LautaroPetaccio LautaroPetaccio commented Mar 21, 2022

This PR moves the computation of the local_content_hash of the item down below the upsertDCLItem method so the local_content_hash gets computed with the changes to the item.
Closes #455

@coveralls
Copy link

coveralls commented Mar 21, 2022

Pull Request Test Coverage Report for Build 2017373438

  • 7 of 7 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 68.381%

Totals Coverage Status
Change from base Build 2006500397: 0.1%
Covered Lines: 2346
Relevant Lines: 3283

💛 - Coveralls


// Compute the content hash of the item to later store it in the DB
attributes.local_content_hash =
attributes?.blockchain_item_id && dbCollection?.contract_address
Copy link
Contributor

Choose a reason for hiding this comment

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

two things

  • is attributes? necessary? seems to always be defined right?
  • shouldn't this be a function with a name? it's also used elsewhere like in buildStandardWearableEntityMetadata

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 didn't understand the question about attributes. I'm using attributes there to have an updated representation of what's going to be inserted into the database to compute its hash.
About the second one, it's similar to the one in buildStandardWearableEntityMetadata, as this one checks that the dbCollection exists, but I've refactored into a function so we can name it.

Copy link
Contributor

Choose a reason for hiding this comment

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

The thing about attributes is that we're using ? to check for a prop, but seems unnecessary given the context.
👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, you're right! I've changed the code to use the function that checks if the item was published, this code is outdated

@LautaroPetaccio LautaroPetaccio merged commit dcc1445 into master Mar 21, 2022
@LautaroPetaccio LautaroPetaccio deleted the fix/hash-computation branch March 21, 2022 18:53
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.

The local content hash is computed with the old item data
3 participants