Standard v2 Cadence 1.0 updates #194

sisyphusSmiling · 2023-10-20T21:40:21Z

Related: #133

Description

This PR updates the repo's Cadence contracts, transactions and scripts as well as supporting test suites for Cadence 1.0 compatibility. As such, it targets the standard-v2 branch where work on v2 of NonFungibleToken is ongoing.

All go tests are passing, but a bug in test account address retrieval prevents a couple of the Cadence test cases from passing.

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

Update the version in package.json when there is a change in the smart contracts

Bumps [github.com/ethereum/go-ethereum](https://github.com/ethereum/go-ethereum) from 1.9.13 to 1.12.1. - [Release notes](https://github.com/ethereum/go-ethereum/releases) - [Commits](ethereum/go-ethereum@v1.9.13...v1.12.1) --- updated-dependencies: - dependency-name: github.com/ethereum/go-ethereum dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]>

Bumps [golang.org/x/net](https://github.com/golang/net) from 0.10.0 to 0.17.0. - [Commits](golang/net@v0.10.0...v0.17.0) --- updated-dependencies: - dependency-name: golang.org/x/net dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]>

…nce tests

joshuahannan · 2023-11-02T14:33:22Z

contracts/utility/FungibleToken.cdc

-    /// This interface is now a general purpose metadata interface because
-    /// a public interface is needed to get metadata, but adding a whole new interface
-    /// for every account to upgrade to is probably too much of a breaking change
-    access(all) resource interface Balance { //: ViewResolver.Resolver {


Why remove this interface?

Thanks for catching that. I didn't mean to remove the interface. I see it's in the current v2 implementation, so not sure how that got removed. I'll make sure to re-include it.

Revisiting this, looks like there is not a Balance interface defined in FT-v2. I think I assumed that it was omitted on purpose, but worth revisiting if not.

contracts/ExampleNFT-v2.cdc

joshuahannan · 2023-11-02T18:17:45Z

contracts/NonFungibleToken-v2.cdc


        /// Function for a direct transfer instead of having to do a deposit and withdrawal
        /// This can and should return false if the transfer doesn't succeed and true if it does succeed
        ///
        access(Withdrawable) fun transfer(id: UInt64, receiver: Capability<&{NonFungibleToken.Receiver}>): Bool {
            pre {
                receiver.check(): "Could not borrow a reference to the NFT receiver"
-                //NonFungibleToken.emitNFTTransfer(id: id, uuid: self.borrowNFTSafe(id: id)?.uuid, from: self.owner?.address, to: receiver.borrow()?.owner?.address, type: self.borrowNFT(id).getType().identifier)
+                self.getIDs().contains(id): "The collection does not contain the specified ID"


This could be a problem because some collections are so large that they can't return the whole list of IDs because it goes over the gas limit. might need to remove this. lets try to get some other opinions though

Good point. FWIW I think it's best to remove it to keep the interface flexible, but worth bringing up in the open house this week

If we want to proceed here, the problem then is in the NonFungibleToken.emitNFTTransfer(..., type: self.borrowNFT(id).getType().identifier). We'll need to determine if we can emit String? otherwise the self.getIDs().contains(id) pre-condition remains necessary. FWIW I'll add the condition back in and leave it as a point of discussion

joshuahannan · 2023-11-02T18:19:46Z

contracts/NonFungibleToken-v2.cdc

        access(Withdrawable) fun transfer(id: UInt64, receiver: Capability<&{NonFungibleToken.Receiver}>): Bool {
            pre {
                receiver.check(): "Could not borrow a reference to the NFT receiver"


I made the return type of this a Bool because I thought it might be nice to let people return false if something went wrong instead of reverting. What do you think about removing all the pre-conditions here to help support that? I guess that would make it so we can't emit the transfer event every time though because it might fail. Not sure what is better

It's a good question. My stance is we remove the pre-condition and proceed with your original idea. That said, I think this is a good question to present at the open house this week and see what others think. For the purpose of this PR, I say we leave it as-is until we get more feedback about it.

scripts/get_collection_length.cdc

scripts/get_collection_length_from_storage.cdc

joshuahannan · 2023-11-02T18:54:54Z

tests/example_nft_tests.cdc

+
+    let expectedCollectionLength = 1
+
+    txExecutor("setup_account_to_receive_royalty.cdc", [admin], [/storage/flowTokenVault], nil, nil)


Can you add a comment about why we need to execute this transaction here before minting?

joshuahannan · 2023-11-02T19:04:44Z

tests/example_nft_tests.cdc

+access(all) fun testGetCollectionIDs() {
+    let expectedCollectionLength = 1
+
+    let actualNFTIDs = scriptExecutor("get_collection_ids.cdc", [
+            recipient.address,
+            /public/cadenceExampleNFTCollection
+        ]) as! [UInt64]? ?? panic("Could not get collection IDs")
+
+    Test.assertEqual(expectedCollectionLength, actualNFTIDs.length)
+}
+
+access(all) fun testGetCollectionLength() {
+    let expectedCollectionLength = 1
+
+    let actualCollectionLength = scriptExecutor("get_collection_length.cdc", [admin.address]) as! Int?
+        ?? panic("Could not get collection length")
+
+    Test.assertEqual(expectedCollectionLength, actualCollectionLength)
+}


Are these basically the same test?

I know we're using different scripts, but we should probably be making sure the IDs are correct in the first one instead of just checking the length

These could probably be consolidated into one test, but they do use different scripts since one now uses .getIDs() and the other uses .getLength().

Not sure how we would validate the id given it's the uuid. Maybe confirming NFT.uuid == NFT.getID() && collection.getIDs().contains(id)?

tests/test_helpers.cdc

…contains(id)

dependabot bot and others added 24 commits September 6, 2023 16:56

remove js packages

03914e9

Merge branch 'master' into standard-v2-updates

21640aa

update NFTForwarding for Cadence 1.0

1a261c0

update NFTForwarding transactions for new implementation

d9ef40e

bump ci flow cli version to Cadence 1.0 pre-release

2d9a34c

fix ExampleNFT-v2 account storage access bug

8014190

update FungibleToken Cadence 1.0 implementation

4311a6c

update flow.json with v2 contract implementations

262c73f

update txns & scripts for Cadence 1.0 + supporting ExampleNFT-v2 Cade…

d872c31

…nce tests

update Makefile & go tests

c6330af

update NFT-v2 to emit events from interface pre/post conditions

ef9b4d7

update ExampleNFT-v2 Cadence tests & go assets

359da96

update ExampleNFT-v2 providerPath

0649cdf

update go tests and supporting txns

b959c5b

update go assets

92526d4

fix test cases to correspond with new contract implementations

42b00b3

add metadata trait test validation to go suite

0f2bb28

update flow.json with emulator aliases

6958b19

fix NFTForwarding.Forwarder conformance to .Receiver & rename events

cf9a423

add NFTForwarding Cadence tests & fix txn bugs

c7aa593

complete NFTForwarding cadence tests + supporting txns & scripts

cb3b555

update .gitignore to include coverage.json

f42de59

sisyphusSmiling added Improvement SC-Eng Issues that we want to see surfaced in SC-Eng ZH Board labels Oct 24, 2023

fix contract.go placeholder variable name

88a73f1

sisyphusSmiling changed the title ~~[WIP] Standard v2 Cadence 1.0 updates~~ Standard v2 Cadence 1.0 updates Oct 31, 2023

sisyphusSmiling marked this pull request as ready for review October 31, 2023 21:08

sisyphusSmiling requested a review from joshuahannan as a code owner October 31, 2023 21:08

sisyphusSmiling self-assigned this Oct 31, 2023

joshuahannan approved these changes Nov 2, 2023

View reviewed changes

sisyphusSmiling added 6 commits November 6, 2023 17:41

update NFT.transfer() pre-conditions on recevier.check() & .getIDs().…

3c50d77

…contains(id)

update instances of Collection.getIDs().length with .getLength()

60eb711

add comments to test_helpers methods

8531ac6

update go assets

4347b53

re-add .transfer() pre-condition on .getIDs().contains(id)

86ca92b

update go assets

3f3d5a7

sisyphusSmiling merged commit 683df24 into standard-v2 Nov 7, 2023
0 of 2 checks passed

sisyphusSmiling deleted the standard-v2-updates branch November 7, 2023 00:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standard v2 Cadence 1.0 updates #194

Standard v2 Cadence 1.0 updates #194

sisyphusSmiling commented Oct 20, 2023 •

edited

Loading

joshuahannan Nov 2, 2023

sisyphusSmiling Nov 6, 2023

sisyphusSmiling Nov 6, 2023

joshuahannan Nov 2, 2023

sisyphusSmiling Nov 6, 2023

sisyphusSmiling Nov 7, 2023 •

edited

Loading

joshuahannan Nov 2, 2023

sisyphusSmiling Nov 6, 2023

joshuahannan Nov 2, 2023

joshuahannan Nov 2, 2023

joshuahannan Nov 2, 2023

sisyphusSmiling Nov 6, 2023


		let expectedCollectionLength = 1

		txExecutor("setup_account_to_receive_royalty.cdc", [admin], [/storage/flowTokenVault], nil, nil)

Standard v2 Cadence 1.0 updates #194

Standard v2 Cadence 1.0 updates #194

Conversation

sisyphusSmiling commented Oct 20, 2023 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sisyphusSmiling Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sisyphusSmiling commented Oct 20, 2023 •

edited

Loading

sisyphusSmiling Nov 7, 2023 •

edited

Loading