Skip to content

Conversation

@joze1239
Copy link
Contributor

No description provided.

@joze1239 joze1239 changed the title Feature/884 batch transfer nft contract 884: NFT batch transfer contract Jan 11, 2022
@joze1239 joze1239 requested a review from sreckosmodis January 11, 2022 11:17
@joze1239
Copy link
Contributor Author

This is not needed anymore :D

@joze1239 joze1239 closed this Jan 18, 2022
@joze1239 joze1239 reopened this Jan 24, 2022
@joze1239
Copy link
Contributor Author

apparently, we need it again :D @sreckosmodis can you check this PR?



for (uint256 index; index < recipients.length; index++) {
ERC1155Base(contractAddress).safeTransferFrom(msg.sender, recipients[index], tokenIds[index], amounts[index], "");
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to check the balance before the transfer?
So another require statement?

Get balance -> require to be more or equal to the amount

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, good point. I will add this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Copy link
Member

Choose a reason for hiding this comment

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

Now there is another point of view 😄
Do we prevent all the transfers just because one of the NFTs has insufficient balance?
If we do that we should at least add the tokenId to the error message 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted balance checking changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants