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

feat: add nft and token linker contracts #144

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

deanamiel
Copy link

PR to add NFT and Token linker contracts and tests to the examples repo.

@npty
Copy link
Member

npty commented Aug 15, 2023

Hey, thanks for the PR 🙌🏻

Can you add a script to deploy and execute like other examples? So, dev can play around with example from the cli.

@deanamiel
Copy link
Author

No problem @npty! And yes definitely, I will add those scripts once we wrap up with audits and releasing governance.

address gateway_,
address gasReceiver_,
address owner_
) ERC721('Axelar NFT Linker', 'ANL') AxelarExecutable(gateway_) Upgradable(owner_) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
) ERC721('Axelar NFT Linker', 'ANL') AxelarExecutable(gateway_) Upgradable(owner_) {
) ERC721('Axelar NFT Linker', 'ANL') AxelarExecutable(gateway_) Upgradable(msg.sender) {

Just use msg.sender for these example contracts for simplicity and avoids breaking compatibility


constructor(address implementationAddress, address owner, bytes memory setupParams) Proxy(implementationAddress, owner, setupParams) {}

// slither-disable-next-line dead-code
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// slither-disable-next-line dead-code

add the same slither/solhint config to this repo?

Copy link
Author

Choose a reason for hiding this comment

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

added solhint/slither configs to ignore these warnings

@@ -1,3 +1,4 @@
require("@nomiclabs/hardhat-ethers");
Copy link
Member

Choose a reason for hiding this comment

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

why not import the toolbox here?

examples/tests/NftLinker.js Outdated Show resolved Hide resolved
examples/tests/NftLinker.js Outdated Show resolved Hide resolved
package.json Outdated
@@ -31,8 +31,10 @@
"uuid": "^8.3.2"
},
"devDependencies": {
"@nomiclabs/hardhat-ethers": "^2.2.3",
Copy link
Member

Choose a reason for hiding this comment

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

why not use the toolbox and remove this and gas reporter?

Copy link
Author

Choose a reason for hiding this comment

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

I removed this, I tried to install toolbox and remove gas reporter but ran into a lot of dependency conflicts between the toolbox ethers and ethers in the dependencies. It seems like we are only using the gas reporter package from the toolbox so maybe importing the entire toolbox at the point is unnecessary?

examples/tests/NftLinker.js Outdated Show resolved Hide resolved
examples/tests/TokenLinker.js Outdated Show resolved Hide resolved
examples/tests/NftLinker.js Outdated Show resolved Hide resolved
package.json Outdated
@@ -31,8 +31,10 @@
"uuid": "^8.3.2"
},
"devDependencies": {
"@nomiclabs/hardhat-ethers": "^2.2.3",
Copy link
Member

Choose a reason for hiding this comment

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

now that tests were removed this shouldn't be needed?

Copy link
Author

Choose a reason for hiding this comment

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

removed

@@ -1,3 +1,4 @@
require('@nomiclabs/hardhat-ethers');
Copy link
Member

Choose a reason for hiding this comment

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

same as below

Copy link
Author

Choose a reason for hiding this comment

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

removed

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