AINFTize contract template, integration test, documentation #3
AINFTize contract template, integration test, documentation #3orth0gonal wants to merge 40 commits into
Conversation
feat: AINFT contract template structure impl.
orth0gonal
added
documentation
There was a problem hiding this comment.
There was a problem hiding this comment.
use only one. yarn.lock or package-lock.json.
| returns (bool) | ||
| { | ||
| // IERC4906 interface added | ||
| return interfaceId == bytes4(0x49064906) || super.supportsInterface(interfaceId); |
There was a problem hiding this comment.
How to distinguish this NFT is AINFT. should we add our own interfaceId on this?
There was a problem hiding this comment.
FYI (might help, but I don't know yet): https://docs.openzeppelin.com/contracts/3.x/api/introspection#IERC1820Registry-interfaceHash-string-
There was a problem hiding this comment.
I agree that we'd better add our own interfaceId. Maybe we have to think deeply about it further, I leave it as future work.
There was a problem hiding this comment.
But in this quarter plan, the marketplace should be searchable all AINFTs. without own interfaceId or without AINFTFactory's managed list, it would be quite tricky.
There was a problem hiding this comment.
Regarding to marketplace, it would be necessary to distinguish AINFT from other ERC721 NFTs, so our own interfaceId should be set ASAP!
There was a problem hiding this comment.
It would be discussed further in #5 . Thanks!
|
|
||
| function setPaymentContract(address paymentPlugin_) public onlyRole(DEFAULT_ADMIN_ROLE) { | ||
| require(_msgSender() == tx.origin && _msgSender() != address(0), "Only EOA can set payment contract."); | ||
| PAYMENT_PLUGIN = paymentPlugin_; |
There was a problem hiding this comment.
I think we need to check the paymentPlugin_ is already pointed to this contract. (because of the callflows)
- AINFT721 created
- AINPayment created (with these AINFT721 address)
- AINFT721 setPaymentContract (with above)
- so we can check this in here.
There was a problem hiding this comment.
Is there any desirable way that setPaymentContract can check whether AINPayment is already pointed to this contract?
The first method I've thought was to make connect function inside AINPayment, which calls AINFT721::setPaymentContract(), but it occurs vulnerability that anyone who deploys AINPayment can change the PAYMENT_PLUGIN.
There was a problem hiding this comment.
I'm going to just add require(paymentPlugin_.isContract()); to check whether paymentPlugin_ is contract address or not. Any good ways are welcomed!
There was a problem hiding this comment.
In my first glance, I was thinking the way by checking paymentPlugin_.ainft value is same with this contract's address. just value comparision. that's all.
| constructor(string memory name_, string memory symbol_, bool isCloned_, address originNFT_) ERC721(name_, symbol_) { | ||
| if (isCloned_) { | ||
| //FIXME: need to use tx.origin? | ||
| _grantRole(DEFAULT_ADMIN_ROLE, tx.origin); |
There was a problem hiding this comment.
what's different between if(isCloned_) { ... /* HERE */ ... } else { ... /* THERE */ ... }.
what's expected tx.origin value whether isCloned_ on/off?
There was a problem hiding this comment.
oh.. it's weird. I don't know why I use if condition. I'll remove it thanks!
| event CloneAINFT721(address indexed originalNFT, address indexed clonedAINFT); | ||
| event CreateAINFT721(address indexed createdAINFT); | ||
|
|
||
| constructor() Ownable() {} |
There was a problem hiding this comment.
should we limit the callable only for contract owner?
There was a problem hiding this comment.
It is a complex problem related to the existence of AINFTFactory contract itself. Let's discuss on #6 !
|
Give us the PTAL sign when you feel comfortable. |
Sorry for the late update, I'm in the process of writing entire design of contract moving on "our Design Doc" format due to a few feedbacks such as hard to understand, should precede design review, etc. I will arrange a design review meeting after the draft finishes ASAP.(Possibly the next week). Our discussion about structure or some issues would be continued on the design meeting. |
5 participants
Description
npx hardhat test test/integrationTest.ts)Need to be Reviewed
contracts/test/integrationTest.ts로직이 살짝 복잡한 관계로 smart contract 작성 경험 있는 개발자 분들, AINFTize와 관련있는 개발자 분들 모두 reviewer로 추가하였으니 시간 나시는 분들은 리뷰해 주시면 많은 도움이 될 것 같습니다!(미리 감사합니다!) 자세한 flow는 README의 call graph를 참조하시면 조금 더 이해하시기 쉽습니다.