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 NFT Bridging Cadence tests #24

Merged
merged 76 commits into from
Apr 3, 2024
Merged

Add NFT Bridging Cadence tests #24

merged 76 commits into from
Apr 3, 2024

Conversation

sisyphusSmiling
Copy link
Contributor

@sisyphusSmiling sisyphusSmiling commented Apr 1, 2024

Related: #16
Stacked against: #20

Description

Adds test cases. Following on from #22 after git issues.


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.
  • Code follows the standards mentioned here.
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

sisyphusSmiling and others added 30 commits March 25, 2024 19:02
@codecov-commenter
Copy link

codecov-commenter commented Apr 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.79%. Comparing base (a17d812) to head (97cdfcd).

Additional details and impacted files
@@                  Coverage Diff                   @@
##           add-serialization      #24       +/-   ##
======================================================
+ Coverage              58.92%   74.79%   +15.86%     
======================================================
  Files                      5       13        +8     
  Lines                    297      599      +302     
======================================================
+ Hits                     175      448      +273     
- Misses                   122      151       +29     

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

@sisyphusSmiling sisyphusSmiling marked this pull request as ready for review April 1, 2024 21:05
@sisyphusSmiling sisyphusSmiling requested a review from a team as a code owner April 1, 2024 21:05
@sisyphusSmiling sisyphusSmiling changed the title Merge test fix base Add NFT Bridging Cadence tests Apr 1, 2024
[contractAddr, contractName, nftID],
signer
)
Test.expect(bridgeResult, Test.beSucceeded())
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a few more lines after this one and for the next test to verify this worked? Could be as simple as using the get ids to make sure the id still exists here in the account on the evm side, and similar for the next test. Also an event emitted check would be good.

Copy link
Collaborator

Choose a reason for hiding this comment

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

These are actually helper functions, used in test cases. The actual tests do contain assertions for checking the IDs etc.
I have added some assertions about checking emitted events: f45ad59

Copy link
Contributor

@aishairzay aishairzay 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!

import "EVM"

/// This contract is intended for testing purposes for the sake of capturing a deployed contract address while native
/// `evm.TransactionExecuted` event types are not available in Cadence testing framework. The deploying account should
Copy link
Collaborator

Choose a reason for hiding this comment

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

Soon enough there will be no need for such workarounds 🙏 . I already have a WIP PR in onflow/cadence-tools#330


var events = Test.eventsOfType(Type<NonFungibleToken.Withdrawn>())
let withdrawnEvent = events[events.length - 1] as! NonFungibleToken.Withdrawn
Test.assertEqual(bridgeAccount.address, withdrawnEvent.from!)
Copy link
Collaborator

@m-Peter m-Peter Apr 3, 2024

Choose a reason for hiding this comment

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

I would like to add some assertions to check the withdrawnEvent.id & depositedEvent.id event fields, but I am not sure how to convert from the erc721ID argument to a Cadence NFT ID. Any ideas @sisyphusSmiling ?

Copy link
Contributor Author

@sisyphusSmiling sisyphusSmiling Apr 3, 2024

Choose a reason for hiding this comment

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

I think the best way to do this would be to look for the subsequent IFlowEVMNFTBridge.BridgedNFTFromEVM event and match the evmID value to erc721. This event will also contain the id value corresponding to the bridged nft.id value which can then be checked in the Withdrawn and Deposited events. This is a bit roundabout, but necessary given the fact evmID is not guaranteed to match nft.id due to different types - UInt256 and UInt64 respectively.

Copy link
Collaborator

@m-Peter m-Peter left a comment

Choose a reason for hiding this comment

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

Nice 👏

@sisyphusSmiling sisyphusSmiling changed the base branch from add-serialization to main April 3, 2024 20:50
@sisyphusSmiling sisyphusSmiling merged commit 5d36089 into main Apr 3, 2024
2 checks passed
@sisyphusSmiling sisyphusSmiling deleted the merge-test-fix-base branch April 26, 2024 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

5 participants