Skip to content
This repository has been archived by the owner on Aug 21, 2023. It is now read-only.

I terminus #53

Closed
wants to merge 5 commits into from
Closed

I terminus #53

wants to merge 5 commits into from

Conversation

peersky
Copy link
Contributor

@peersky peersky commented Jul 4, 2022

Changes

How to test these changes?

Related issues

#13

@peersky peersky requested a review from zomglings July 4, 2022 11:30
@peersky peersky linked an issue Jul 4, 2022 that may be closed by this pull request
@peersky
Copy link
Contributor Author

peersky commented Jul 4, 2022

cd build/contracts
diff  <(jq --sort-keys .abi TerminusFacet.json) <(jq --sort-keys .abi ITerminus.json)        
3,7d2
<     "inputs": [],
<     "stateMutability": "nonpayable",
<     "type": "constructor"
<   },
<   {
743c738
<         "name": "poolID",
---
>         "name": "id",
 diff  <(jq --sort-keys .abi ERC1155WithTerminusStorage.json) <(jq --sort-keys .abi IERC1155WithTerminusStorage.json) 
3,7d2
<     "inputs": [],
<     "stateMutability": "nonpayable",
<     "type": "constructor"
<   },
<   {
346c341
<         "name": "poolID",
---
>         "name": "id",

Copy link
Member

@zomglings zomglings left a comment

Choose a reason for hiding this comment

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

Terminus interface has changed in the most recent releases. This needs to be updated.

Given that Terminus is still in flux, I would rather do this kind of work using codegen than manually maintaining ITerminus.sol. @peersky : Do you want to build the code generator?

It would also be nice to have a tool which could test an interface against an actual ABI to make sure that the interface is complete (or as complete as it needs to be).

import "@openzeppelin-contracts/contracts/token/ERC1155/extensions/IERC1155MetadataURI.sol";

interface IERC1155WithTerminusStorage is IERC1155, IERC1155MetadataURI {
function isApprovedForPool(uint256 poolID, address operator)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this interface. Might as well roll this into ITerminus.

pragma solidity ^0.8.0;
import "./IERC1155WithTerminusStorage.sol";

interface ITerminus is IERC1155WithTerminusStorage {
Copy link
Member

Choose a reason for hiding this comment

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

I would rather this was codegen'd instead of created manually.

@zomglings
Copy link
Member

Tests are passing on my machine.

@peersky
Copy link
Contributor Author

peersky commented Jul 7, 2022

@zomglings there already is hh plugin that does such generation: https://github.com/dmihal/hardhat-interface-generator if we plan to migrate to hardhat than we can use it, otherwise script is quite simple to reapply to our realities

Nevertheless, in current PR there are points where source code has to be modified anyway so purely generating interface file is not enough.

@zomglings
Copy link
Member

Stale.

@zomglings zomglings closed this Oct 31, 2022
@zomglings zomglings deleted the ITerminus branch October 31, 2022 09:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

ITerminus - we need to define the terminus interface separately
2 participants