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

[r2r] update deps and solc version, add ERC721 and ERC1155 support #2

Merged
merged 23 commits into from
Jan 22, 2024

Conversation

laruh
Copy link
Member

@laruh laruh commented Nov 20, 2023

  • update deps
  • update solidity version from ^0.5.0 to ^0.8.20
  • impl ERC721 functions for EtomicSwap contract
  • ERC721 tests
  • impl ERC1155 functions for EtomicSwap contract
  • ERC1155 tests

Useful links

https://github.com/OpenZeppelin/openzeppelin-contracts/tree/master/contracts/token/ERC1155
https://github.com/OpenZeppelin/openzeppelin-contracts/tree/master/contracts/token/ERC721

Notes:

  • there are simple erc721 and erc1155 holders that can be inherited by the EtomicSwap contract, but I decided to keep onERC1155Received, onERC1155BatchReceived, onERC721Received functions external, as its more gas-efficient.

  • to test changes please follow the setup dev env guide in readme

deps changes

"chai": "^4.1.2" -> "chai": "^4.3.10"

"ganache-cli": "^6.2.5" -> "ganache": "^7.9.1"

"openzeppelin-solidity": "https://github.com/OpenZeppelin/openzeppelin-solidity.git#release-v2.1.0-solc-0.5" -> "@openzeppelin/contracts": "^5.0.0"

"sol-merger": "^0.1.0" -> "sol-merger": "^4.4.0"

"web3": "^1.0.0-beta.27" -> "web3": "^4.2.2"

@laruh laruh changed the base branch from dev to master November 20, 2023 05:23
@shamardy shamardy changed the base branch from master to dev November 21, 2023 19:16
@laruh laruh changed the title Update dependencies and sol version [wip] update deps and solc version, add ERC721 support Nov 23, 2023
@laruh laruh changed the title [wip] update deps and solc version, add ERC721 support [r2r] update deps and solc version, add ERC721 and ERC1155 support Nov 24, 2023
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

First review iteration, few comments/questions.

test/EtomicSwap.js Outdated Show resolved Hide resolved
test/EtomicSwap.js Outdated Show resolved Hide resolved
contracts/EtomicSwap.sol Show resolved Hide resolved
contracts/EtomicSwap.sol Show resolved Hide resolved
contracts/EtomicSwap.sol Outdated Show resolved Hide resolved
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

Few more notes 🙂

contracts/EtomicSwap.sol Outdated Show resolved Hide resolved
contracts/EtomicSwap.sol Outdated Show resolved Hide resolved
assert.equal(tokenOwnerBeforeReceiverSpend, this.swap.address);

// Attempt to spend with invalid secret - should fail
await this.swap.receiverSpendErc721(id, zeroAddr, this.erc721token.address, tokenId, accounts[0], { from: accounts[1] }).should.be.rejectedWith(EVMThrow);
Copy link
Member

Choose a reason for hiding this comment

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

zeroAddr is 20 bytes instead of 32, please use 32 bytes argument for 100% correctness 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

added invalidSecretHex

test/EtomicSwap.js Outdated Show resolved Hide resolved
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

One question

test/EtomicSwap.js Outdated Show resolved Hide resolved
@shamardy shamardy self-requested a review December 5, 2023 18:12
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

Great work! 🔥

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