En 9146 nfts like esdt unit tests #2899

bogdan-rosianu · 2021-03-11T12:00:36Z

Unit tests for new added ESDT NFT components

…m/ElrondNetwork/elrond-go into EN-9146-nfts-like-esdt-unit-tests

sasurobert · 2021-03-15T12:12:17Z

integrationTests/vm/esdt/esdtNft_test.go

+
+	checkNftData(t, nodes[1].OwnAccount.Address, nodes, tokenIdentifier, &nftMetaData, false)
+
+	// increase quantity


change comment to decrease quantity

sasurobert · 2021-03-15T12:13:06Z

integrationTests/vm/esdt/esdtNft_test.go

+	time.Sleep(time.Second)
+
+	// the token data is removed from trie if the quantity is 0, so we should not find it
+	checkNftData(t, nodes[1].OwnAccount.Address, nodes, tokenIdentifier, &nftMetaData, true)


why do you send nftMetaData over if you want to find it empty ?

sasurobert · 2021-03-15T12:17:46Z

integrationTests/vm/esdt/esdtNft_test.go

+
+	///////////------- send token issue
+
+	issueNFT(nodes, core.SemiFungibleESDT)


this part of issue and set special roles may be taken out into a separate function. like prepareESDTNFTData and gets some arguments.

done. this prepare function has a lot of arguments, but with this refactoring I managed to reduce the code duplication 👍

sasurobert · 2021-03-15T13:06:20Z

integrationTests/vm/esdt/esdtNft_test.go

+	tokenIdentifierPlusNonce = append(tokenIdentifierPlusNonce, big.NewInt(0).SetUint64(1).Bytes()...)
+	esdtData := getESDTTokenData(t, address, nodes, string(tokenIdentifierPlusNonce))
+
+	if shouldNotFound {


I think this bool is not needed. you can use args.quantity == 0

done. nice idea 👍

sasurobert · 2021-03-15T13:48:09Z

integrationTests/vm/esdt/esdtNft_test.go

+	)
+}
+
+func TestESDTSemiFungibleTokenTransferCrossShard(t *testing.T) {


function too long. make smaller reusable functions.

sasurobert · 2021-03-15T13:48:23Z

integrationTests/vm/esdt/esdtNft_test.go

+	)
+}
+
+func TestESDTSemiFungibleTokenCreateAddAndBurn(t *testing.T) {


function too long

extracted the functionality for preparing

…nit-tests # Conflicts: # go.mod # go.sum

- changed transfer unit tests to work with smart contracts calls

…m/ElrondNetwork/elrond-go into EN-9146-nfts-like-esdt-unit-tests � Conflicts: � process/smartContract/builtInFunctions/esdtNFTTransfer_test.go

…nit-tests

…sc-tests tNFT Transfer create role integration test

SebastianMarian · 2021-03-17T16:27:11Z

integrationTests/vm/esdt/esdtNft_test.go

+
+	nonceArg := hex.EncodeToString(big.NewInt(0).SetUint64(1).Bytes())
+	quantityToSend := int64(1)
+	quantityToBurnArg := hex.EncodeToString(big.NewInt(quantityToSend).Bytes())


quantityToTransferArg

SebastianMarian · 2021-03-17T16:31:13Z

integrationTests/vm/esdt/esdtNft_test.go

+	}
+
+	nonceArg := hex.EncodeToString(big.NewInt(0).SetUint64(1).Bytes())
+	quantityToSend := int64(1)


quantityToTransfer

SebastianMarian · 2021-03-17T19:09:30Z

process/smartContract/builtInFunctions/esdtNFTAddQuantity_test.go

+	require.Equal(t, newGasCost, eqf.funcGasCost)
+}
+
+func TestEsdtNFTAddQuantity_ProcessBuiltinFunctionErrorOncheckESDTNFTCreateBurnAddInput(t *testing.T) {


SebastianMarian · 2021-03-17T19:22:51Z

process/smartContract/builtInFunctions/esdtNFTBurn_test.go

+	require.Equal(t, newGasCost, ebf.funcGasCost)
+}
+
+func TestEsdtNFTBurnFunc_ProcessBuiltinFunctionErrorOncheckESDTNFTCreateBurnAddInput(t *testing.T) {


SebastianMarian · 2021-03-17T20:03:09Z

process/smartContract/builtInFunctions/esdtNFTCreate.go

-	if err != nil {
-		return nil, err
+	if isNew {
+		return nil, process.ErrNewNFTDataOnSenderAddress


return esdtData, process.ErrNewNFTDataOnSenderAddress ? or return nil, true, nil -> at line 222 ?
(esdtData is not used in the caller side if isNew is true)

no. on sender it should error. as you must have it in order to transfer / burn or anything.

it is correct as it is.

SebastianMarian · 2021-03-17T20:53:54Z

process/smartContract/builtInFunctions/esdtNFTCreateRoleTransfer.go

@@ -214,9 +216,6 @@ func (e *esdtNFTCreateRoleTransfer) executeTransferNFTCreateChangeAtNextOwner(
 	acntDst state.UserAccountHandler,
 	vmInput *vmcommon.ContractCallInput,
 ) error {
-	if check.IfNil(acntDst) {


This check should be kept, otherwise it will panic in saveLatestNonce method called below

it is checked above. on line 71

SebastianMarian · 2021-03-17T21:03:30Z

process/smartContract/builtInFunctions/esdtNFTCreateRoleTransfer_test.go

+	destAcc, _ := e.accounts.LoadAccount(addr)
+	userAcc := destAcc.(state.UserAccountHandler)
+	nonce, _ := getLatestNonce(userAcc, tokenID)
+	assert.Equal(t, nonce, expectedNonce)


assert.Equal(t, expectedNonce, nonce)

SebastianMarian · 2021-03-17T21:33:34Z

process/smartContract/builtInFunctions/esdtRoles.go

 	}

-	roles := &esdt.ESDTRoles{
-		Roles: make([][]byte, 0),
-	}
 	if len(marshaledData) == 0 {


This condition could be moved with || in L116

SebastianMarian · 2021-03-17T21:42:51Z

vm/systemSmartContracts/esdt_test.go

+	require.Equal(t, vmcommon.Ok, retCode)
+}
+
+func TestEsdt_SetSpecialRoleSemiNFTShouldErr(t *testing.T) {


SFT or SemiFT instead SemiNFT

iulianpascalau · 2021-03-18T09:19:33Z

integrationTests/vm/esdt/esdtNFTSCs_test.go

@@ -0,0 +1 @@
+package esdt


empty file?

it will be filled in next prs.

iulianpascalau · 2021-03-18T09:26:40Z

process/smartContract/hooks/blockChainHook.go

@@ -371,7 +372,7 @@ func (bh *BlockChainHookImpl) ProcessBuiltInFunction(input *vmcommon.ContractCal
 		}
 	}

-	if !check.IfNil(dstAccount) {
+	if !check.IfNil(dstAccount) && !bytes.Equal(input.CallerAddr, input.RecipientAddr) {


I think this will get clearer if we write it as:

shouldSaveDestination := !check.IfNil(dstAccount) && !bytes.Equal(sndAccount.AddressBytes(), dstAccount.AddressBytes) if shouldSaveDestination{ ....

I would leave as it is.

iulianpascalau · 2021-03-18T09:27:07Z

testscommon/txDataBuilder/builder.go

@@ -159,6 +159,11 @@ func (builder *txDataBuilder) CanBurn(prop bool) *txDataBuilder {
 	return builder.Str("canBurn").Bool(prop)
 }

+// CanTransferNFTCreateRole appends "canTransferNFTCreateRole" followed by the provided boolean value.
+func (builder *txDataBuilder) CanTransferNFTCreateRole(prop bool) *txDataBuilder {


bogdan-rosianu added 4 commits March 11, 2021 11:12

EN-9146: first unit tests

dfb6292

tests for esdt nft add quantity

250d728

added more checks

0d1530d

tests for esdt nft burn

85fa6bc

bogdan-rosianu added the type:feature New feature or request label Mar 11, 2021

bogdan-rosianu assigned iulianpascalau and bogdan-rosianu Mar 11, 2021

bogdan-rosianu and others added 4 commits March 12, 2021 18:18

integration tests + small fixes

adcddb6

- added unit tests for transfer NFT

2cc4a88

Merge branch 'EN-9146-nfts-like-esdt-unit-tests' of https://github.co…

6e0462e

…m/ElrondNetwork/elrond-go into EN-9146-nfts-like-esdt-unit-tests

fix/refactored two tests

f0a2fdd

sasurobert self-requested a review March 15, 2021 12:06

bogdan-rosianu and others added 2 commits March 15, 2021 15:11

int tests

d4e367d

new arwen. corrected save and load account

f0d866c

sasurobert requested changes Mar 15, 2021

View reviewed changes

sasurobert and others added 14 commits March 15, 2021 15:49

Merge branch 'feat/eip-esdt-local-mint' into EN-9146-nfts-like-esdt-u…

d883caf

…nit-tests # Conflicts: # go.mod # go.sum

new arwen

460a3e4

added test for revert esdt

1d02638

- added unit tests for create

a625f3e

- changed transfer unit tests to work with smart contracts calls

Merge branch 'EN-9146-nfts-like-esdt-unit-tests' of https://github.co…

b3a252d

…m/ElrondNetwork/elrond-go into EN-9146-nfts-like-esdt-unit-tests � Conflicts: � process/smartContract/builtInFunctions/esdtNFTTransfer_test.go

- fixed unit tests

9fe36dc

new unit tests for factory

12b4e2e

unit tests for constructor

c82e120

added new unit tests

7ca753a

added new unit tests for role transfer

53ea5ce

new unit tests for esdt sytem sc

6d51ed1

small fix

6896dc5

fix after review

0d68a04

unit tests on esdt system sc transfer create ownership

a68336c

sasurobert marked this pull request as ready for review March 16, 2021 19:30

sasurobert changed the title ~~[WIP] En 9146 nfts like esdt unit tests~~ En 9146 nfts like esdt unit tests Mar 16, 2021

sasurobert previously approved these changes Mar 16, 2021

View reviewed changes

SebastianMarian self-requested a review March 16, 2021 20:06

sasurobert added 3 commits March 17, 2021 10:15

Merge branch 'feat/eip-esdt-local-mint' into EN-9146-nfts-like-esdt-u…

343a39e

…nit-tests

transfer create role integration test

36f97cc

Merge pull request #2912 from ElrondNetwork/EN-9417-nfts-integration-…

3b9061c

…sc-tests tNFT Transfer create role integration test

sasurobert dismissed their stale review via 3b9061c March 17, 2021 12:00

iulianpascalau assigned sasurobert Mar 17, 2021

sasurobert previously approved these changes Mar 17, 2021

View reviewed changes

blokcchainhook fix and local mint and burn through sc

fb90c18

sasurobert dismissed their stale review via fb90c18 March 17, 2021 16:34

SebastianMarian reviewed Mar 17, 2021

View reviewed changes

fix after review

568972f

sasurobert approved these changes Mar 18, 2021

View reviewed changes

iulianpascalau reviewed Mar 18, 2021

View reviewed changes

SebastianMarian approved these changes Mar 18, 2021

View reviewed changes

sasurobert merged commit e771ab4 into feat/eip-esdt-local-mint Mar 18, 2021

sasurobert deleted the EN-9146-nfts-like-esdt-unit-tests branch March 18, 2021 18:49


		checkNftData(t, nodes[1].OwnAccount.Address, nodes, tokenIdentifier, &nftMetaData, false)

		// increase quantity


		///////////------- send token issue

		issueNFT(nodes, core.SemiFungibleESDT)

En 9146 nfts like esdt unit tests #2899

En 9146 nfts like esdt unit tests #2899

Conversation

bogdan-rosianu commented Mar 11, 2021

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment