Improve Solidity usability #277
Conversation
| uint256 indexed oldMaxSupply, | ||
| uint256 indexed newMaxSupply |
There was a problem hiding this comment.
Why are these indexed? I don't think you need filtered search on these parameters, henceforth this only adds unnecessary gas cost.
|
|
||
| event RemoveMinter(address indexed oldMinter); | ||
|
|
||
| event SetBaseURI(string indexed oldUri, string indexed newUri); |
There was a problem hiding this comment.
indexed here doesn't make much sense as well.
- You don't need to filter events for specific uri values.
- Since they are strings, not the string value but its hash will be stored, so you wouldn't be able to retrieve the values from the event log.
| event RemoveWhiteListedToken(address indexed token); | ||
|
|
||
| event WhiteListTokens(address[] indexed token); |
There was a problem hiding this comment.
Since whitelist is one word, it shouldn't be WhiteList but Whitelist
|
|
||
| /// @title Proxy | ||
| /// @dev Proxy contract mostly based on https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/proxy/Proxy.sol | ||
| contract NativeProxy is ERC1967 { |
There was a problem hiding this comment.
This is a proxy for NativeNftSale contract right? I would propose changing the name to NativeNftSaleProxy, so it's more explanatory and consistent with the Proxy naming
|
|
||
| /// @title Proxy | ||
| /// @dev Proxy contract mostly based on https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/proxy/Proxy.sol | ||
| contract Proxy is ERC1967 { |
There was a problem hiding this comment.
This is a proxy for Erc20NftSale contract right? I would propose changing the name to Erc20NftSaleProxy, so it's more explanatory and consistent with the Proxy naming
| // Array of deposited currencies | ||
| ERC20[] public depositedCurrencies; | ||
|
|
||
| // mapping from token address to amount |
There was a problem hiding this comment.
That's not true, it's a mapping from token address to boolean representing if this token has been deposited
| // mapping from token address to amount | ||
| mapping(ERC20 => bool) public depositedCurrenciesMap; | ||
|
|
||
| // Array of supported currencies |
There was a problem hiding this comment.
Comments like these feel really unnecessary 😅
SebastienGllmt
force-pushed
the
improve-script-types
branch
from
4f90a2f to
c0b64e7
Compare
Right now deploying and interacting with the Solidity contracts for Paima is a pain. The old setup was like this:
./paima-engine contractsCLIThe new option is like this:
hardhat-interactor custom Paima tasks)Some tasks off the top of my head that still need to be done
paima contractscommandtypechain-typesin the NPM packageevm-contractsfolder (I don't know why it doesn't like this despite being fine with the old code)evm-contractsfolder.solfilespaima-l2-contractandnftfoldersevm-contractspackage to NPMRelated PRs that are ongoing