Add EVM-native cross-vm NFT interfaces & custom cross-vm registration support #173
Conversation
* update to core-contracts v1.6.0 * add deploy utils transaction and update deps * refactor go pkg * update Makefile and require correct package * add accessor deploy transaction and make sure it is included in templateS * fix bug in import replacement * fix accessor replace
sisyphusSmiling
changed the base branch from
main
to
gio/remove-erc721addr-from-locker
|
@sisyphusSmiling Can you also make sure that you pull the latest changes from |
|
|
||
| access(all) | ||
| fun testRegisterAgainFails() { | ||
| Test.reset(to: snapshot) |
There was a problem hiding this comment.
Just another comment to save code. Maybe we could take a snapshot after we register in the previous test also and then just reset to that so we don't have to copy all the same code here
| var addrRequiresOnboarding = evmAddressRequiresOnboarding(erc721AddressHex) | ||
| ?? panic("Problem getting onboarding requirement by address") | ||
| var typeRequiresOnboarding = typeRequiresOnboardingByIdentifier(exampleCadenceNativeNFTIdentifier) | ||
| ?? panic("Problem getting onboarding requirement by identifier") |
There was a problem hiding this comment.
Where was the custom erc721 contract deployed? I'm not seeing it
There was a problem hiding this comment.
The ERC721 bytecode is passed on deployment of the ExampleCadenceNativeNFT contract and deployed within that contract's init.
There was a problem hiding this comment.
It seems like you are still working on adding more test cases, so I assume you were planning on adding these, but it would be nice to have a test case for each pre-condition and assertion in the register cross-vm nft function. A few examples are verifying that the type checks work for non-NFTs, trying to update a bridge-defined contract, trying to register contracts that don't actually point to each other from either side, or trying to register a cross-VM nft for a cadence contract that has been deployed but the solidity contract has not been configured properly, but there are probably more
There was a problem hiding this comment.
Similar comments as the cadence native tests. Seems like there are a decent amount more edge cases to test, but I'm sure you're planning on getting to it
Co-authored-by: Joshua Hannan <[email protected]>
sisyphusSmiling
force-pushed
the
gio/add-evm-native-interfaces
branch
from
3f8b8f6 to
2e3245a
Compare
|
oh, also can you add the new contracts to the |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## flip-318 #173 +/- ##
===========================================
Coverage ? 79.13%
===========================================
Files ? 23
Lines ? 1337
Branches ? 0
===========================================
Hits ? 1058
Misses ? 279
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@joshuahannan updated go package files in c922464 and ba61ad3 and I got CI passing |
Closes: #167 #144 #145 #146
Description
registerCrossVMNFTif detected. Note that EVM-native cross-VM NFTs which require anNFTFulfillmentMinterCapability will revert on permissionless onboarding. This is viewed as preferential to onboarding via the permissionless path and later requiring an custom association as cross-VM conformance clearly indicates an intention by the project to onboard as a cross-VM NFT.For contributor use:
mainbranchFiles changedin the Github PR explorer