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

Adds getIDs() to NFT program recovery #6513

Merged
merged 2 commits into from
Oct 3, 2024
Merged

Adds getIDs() to NFT program recovery #6513

merged 2 commits into from
Oct 3, 2024

Conversation

joshuahannan
Copy link
Member

Closes #6450

Adds an implementation of NonFungibleToken.Collection.getIDs() to the recovered program for unmigrated NFT contracts.

This will allow projects that didn't migrate to be able to query broken collections and recreate ownership with a newly deployed contract.

@joshuahannan
Copy link
Member Author

Are there any other changes I need to make for this? I couldn't see if there were tests anywhere for this that I needed to update.

@codecov-commenter
Copy link

codecov-commenter commented Oct 1, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 41.26%. Comparing base (21cfe5a) to head (76678dc).
Report is 392 commits behind head on master.

Files with missing lines Patch % Lines
fvm/environment/program_recovery.go 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6513      +/-   ##
==========================================
- Coverage   41.28%   41.26%   -0.02%     
==========================================
  Files        2030     2030              
  Lines      145865   145868       +3     
==========================================
- Hits        60221    60197      -24     
- Misses      79419    79446      +27     
  Partials     6225     6225              
Flag Coverage Δ
unittests 41.26% <0.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@janezpodhostnik
Copy link
Contributor

I don't think there are any tests testing this specifically. This looks like the only location that needs to change.

Someone from @onflow/cadence should take a look as well.

Deploying this requires an HCU.

@joshuahannan
Copy link
Member Author

Do we have an HCU planned for the near future that we can slot this into? There is no rush, but it would be nice to get it in relatively soon because some developers whose contracts broke are asking for it

@turbolent
Copy link
Member

Yes, there's an HCU planned. Please connect with @Kay-Zee @vishalchangrani

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Looks good! Can you please add a test?

@joshuahannan
Copy link
Member Author

@turbolent Where are the tests for this?

@turbolent
Copy link
Member

@turbolent Where are the tests for this?

At the moment there is only TestContractCheckingMigrationProgramRecovery in the Cadence 1.0 migration code, which only tests FT AFAICS. There are no tests in FVM yet and it would be great to add some. Maybe @onflow/flow-cadence-execution can help with that

@joshuahannan
Copy link
Member Author

Yeah, I wouldn't know where to start with that so I could definitely use some help

@joshuahannan joshuahannan added this pull request to the merge queue Oct 3, 2024
Merged via the queue into master with commit 3beefb8 Oct 3, 2024
55 checks passed
@joshuahannan joshuahannan deleted the nft-getids branch October 3, 2024 21:15
vishalchangrani added a commit that referenced this pull request Oct 3, 2024
* add getIDs to program recover

* fix formatting

---------

Co-authored-by: Josh Hannan <hannanjoshua19@gmail.com>
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.

Allow calling getIDs() in broken NFT contracts
4 participants