Refactoring

[mihaisc]

We need a better library. The goal is for js developers to have a good experience.

Topics:

• do all those classes make sense?
• we need to restructure the files. We copied the structure from the contracts, we don't want that
• we need better flows/processes in place for building the packages so we can have an small optimized library

[miquelcabot]

First proposals for the refactoring:

1. Refactor folder names and file position in folders. \pools folder must be renamed
2. Move ConfigHelper from to \utils to \config
3. Move interface types to \@types
4. Simplify imports in \src files
5. Are the ABI parameters necessary in Class constructors? Or can we use the default ABI?
6. Move getErcCreationParams() function to NFTFactory. It’s only used in this class
7. Move getFreOrderParams() function to Datatoken. It’s only used in this class
8. Move getFreCreationParams() function to Datatoken. It’s only used in this class
9. Move getPoolCreationParams() function to Datatoken. It’s only used in this class
10. Move getData() function to Provider. It’s only used in this class
11. Move all \utils\ConversionTypeHelper functions to Provider. They are only used in this class
12. Move all \utils\PoolHelper functions to Pool. They are only used in this class ~13. Remove generateDid() functions. It’s not used~
13. Remove fetchData() functions. It’s not used ~15. Remove downloadFileBrowser() functions. It’s not used~
14. Remove postWithHeaders() functions. It’s not used
15. Remove postData() functions. It’s not used
16. Move downloadFile() function to /utils/General ~19. Add decimals() function to /utils/TokenUtils.ts~ ~20. Add mint() function to /utils/TokenUtils.ts~

Perhaps 19 and 20 can be done in a different Pull Request.

[mihaisc]

5. Not sure what the use of that was. If one changes the contracts they would need a changed library. @alexcos20 do you remember what's the point of ABI ?
6. It is used in the market
7. Used in the market

[miquelcabot]

@mihaisc Do you refer to 6, 7 or 13, 14, 15? I have reviewed it and yes, generateDid(), fetchData() and downloadFileBrowser() are used in the Market

[mihaisc]

Yes, sry, i am refering to 13 to 17, don't need to remove them

[miquelcabot]

More proposals: 21. Put all smart contract related classes in a \contracts folder 22. Create two parent smart contract classes, SmartContract and SmartContractWithAddress from which all smart contract classes will derive from them. These classes will have the address, abi, web3, config, network and Contract properties

[miquelcabot]

More proposals: 23. Unify naming ERC20 --> Datatoken. For example, in Erc20CreateParams --> DatatokenCreateParams, createNftErc20WithPool() --> createNftAndDatatokenWithPool()...

• Public functions:
  • NFTFactory.createNftWithErc20() --> NFTFactory.createNftWithDatatoken()
  • NFTFactory.createNftErc20WithPool() --> NFTFactory.createNftWithDatatokenWithPool()
  • NFTFactory.createNftErc20WithFixedRate() --> NFTFactory.createNftWithDatatokenWithFixedRate()
  • NFTFactory.createNftErc20WithDispenser() --> NFTFactory.createNftWithDatatokenWithDispenser()
  • Datatoken.isERC20Deployer() --> Datatoken.isDatatokenDeployer()
  • NFT.createErc20()--> NFT.createDatatoken()
  • NFT.addErc20Deployer()--> NFT.addDatatokenDeployer()
  • NFT.removeErc20Deployer()--> NFT.removeDatatokenDeployer()
  • NFT.isErc20Deployer()--> NFT.isDatatokenDeployer()

25. Unify naming ERC721 --> Nft. For example, in ERC721Factory --> NftFactory...

[mihaisc]

@kremalicious @alexcos20 should we use dataNft in code as well , instead of just Nft ?

[miquelcabot]

More proposals: 25. Change the estimategas() specific functions. Instead of using a specific function for every task, use a parameter in the functions that let calculate the estimated gas instead of executing the transaction. For example, with this function signature:

public async addNFTTemplate<G extends boolean = false>(
    address: string,
    templateAddress: string,
    estimateGas?: G
  ): Promise<G extends false ? TransactionReceipt : number> {

By default, it executes the transaction and returns a TransactionReceipt value. But if we set the estimateGas parameter to true, it returns a number representing the gas cost, without executing the transaction.

[miquelcabot]

More proposals: 26. Review the use of try... catch blocks in the class methods. Not all smart contract related classes follow the same pattern.

[miquelcabot]

More proposals: 27. General renaming to unify naming conventions.

• Public functions:
  • FixedRateExchange.buyDT() --> FixedRateExchange.buyDatatokens()
  • FixedRateExchange.sellDT() --> FixedRateExchange.sellDatatokens()
  • FixedRateExchange.getDTSupply() --> FixedRateExchange.getDatatokenSupply()
  • FixedRateExchange.getBTSupply() --> FixedRateExchange.getBasetokenSupply()
  • FixedRateExchange.calcBaseInGivenOutDT() --> FixedRateExchange.calcBaseInGivenDatatokensOut()
  • FixedRateExchange.getAmountBTOut() --> FixedRateExchange.getAmountBasetokensOut()
  • FixedRateExchange.collectBT() --> FixedRateExchange.collectBasetokens()
  • FixedRateExchange.collectDT() --> FixedRateExchange.collectDatatokens()
  • SideStaking.getBaseToken() --> SideStaking.getBasetoken()
  • SideStaking.getBaseTokenBalance() --> SideStaking.getBasetokenBalance()
  • SideStaking.getvestingEndBlock() --> SideStaking.getVestingEndBlock()
  • SideStaking.getvestingAmount() --> SideStaking.getVestingAmount()
  • SideStaking.getvestingLastBlock() --> SideStaking.getVestingLastBlock()
  • SideStaking.getvestingAmountSoFar() --> SideStaking.getVestingAmountSoFar()
  • Router.buyDTBatch() --> Router.buyDatatokenBatch()

[miquelcabot]

More proposals: 28. Review try...catch clauses. There are a lot of try...catch clauses that hide the error, and this doesn't give us the ability to ever fix or react to said error. We can see an example of this in #1468.

[miquelcabot]

29. Add functions for complex processes. For example, for Compute to Data. Sometimes these processes call the same functions, and they can be simplified with a unique function