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

Add CrossVM views #243

Merged
merged 9 commits into from
Feb 12, 2025
Merged

Add CrossVM views #243

merged 9 commits into from
Feb 12, 2025

Conversation

sisyphusSmiling
Copy link
Collaborator

@sisyphusSmiling sisyphusSmiling commented Feb 6, 2025

Closes: #242
Related: onflow/flow-evm-bridge#164

Description


For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Re-reviewed Files changed in the Github PR explorer

Copy link
Member

@joshuahannan joshuahannan left a comment

Choose a reason for hiding this comment

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

Would you be able to add examples of these to ExampleNFT and add some tests?

contracts/CrossVMMetadataViews.cdc Outdated Show resolved Hide resolved
contracts/CrossVMMetadataViews.cdc Outdated Show resolved Hide resolved
contracts/CrossVMMetadataViews.cdc Outdated Show resolved Hide resolved
contracts/CrossVMMetadataViews.cdc Outdated Show resolved Hide resolved
@sisyphusSmiling sisyphusSmiling marked this pull request as ready for review February 10, 2025 21:14
@sisyphusSmiling
Copy link
Collaborator Author

@joshuahannan I've added implementation into ExampleNFT and tests. lmk if you think those are sufficient.

Cadence tests are passing locally, but CI is failing on the go tests. I think I'll have to get the EVM contract into the go test env as they're failing locally as well.

We can also add more use case specific examples for Cadence- and EVM-native NFT collections once the bridge changes are complete.

@sisyphusSmiling
Copy link
Collaborator Author

sisyphusSmiling commented Feb 12, 2025

I've removed the original example view resolution in ExampleNFT which attempted to serialize using the SerializeMetadata contract to try and unblock myself here.

Looks like CI is failing due to the changes in the ExampleNFT. When emulator is bootstrapped, it deploys ExampleNFT (ref) and since the contract in this repo now requires two new imports, the bootstrap fails. Not sure how to get around this @joshuahannan as this is a bit circular.

tbh I'm surprised emulator includes a non-critical contract when setting up the environment.

Copy link
Member

@joshuahannan joshuahannan left a comment

Choose a reason for hiding this comment

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

Yeah, this happens sometimes. We'll just need to tag a prerelease based on this branch and use that in flow-go, then when the emulator is updated, we can merge this.

We need to do that anyway with my core contracts PR. I'm sure there is a better way around this, but I don't know either.

@joshuahannan
Copy link
Member

Another option would be to add a separate contract getter to the contracts package to get the new contract, and use that to test. Probably should do that actually so we can still make sure the go tests are passing

@joshuahannan joshuahannan merged commit 66ce9ca into master Feb 12, 2025
2 checks passed
@joshuahannan joshuahannan deleted the gio/add-cross-vm-views branch February 12, 2025 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Add CrossVM views
2 participants