-
Notifications
You must be signed in to change notification settings - Fork 65
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
Make this into a library #9
Comments
I need this also and as far as I know it's also not in openzeppelin (just SafeMath which is less than this) so I want to contribute this (see Pullrequest linked above). Please admin like @rainbreak review and approve (personal mention because in this repo seems to be little activity...) |
Using this as a library would mean having the cost of DELEGATECALL (700 gas) on every arithmetic operation. Have you considered this? [not necessarily true, see below] |
That's right (a test showed me gasUsed can be in practice even higher). But I can see your point. Should I copy&paste the library code into a DSMathC.sol (C for contract L for library)? Then everyone can decide. Downside is that two files have to be maintained I especially neeeded the wmul() func and for me the best thing would be to have it in openzeppelin. I hope this can be a next step... |
You have a good point about the cost of DELEGATECALL. As @JohannesMayerConda alluded to, more complex projects can't fit all functionality into a single contract due to the block gas limit, and have to be split up into multiple contracts. Having each one inherit ds-math is cumbersome. Having the option for both seems like a no-brainer. |
I didn't realize that ds-math is part of DappHub and tests can be executed via https://dapp.tools/ (see PullRequest discussion). My PullRequest would change this library into a truffle project which doesn't fit into the system and was therefore rejected (which I totally understand). I don't know if this would be an easy change to fit into the system but as @rainbreak said gas would also increase and isn't desired. I feel like adapting my Pullrequest to adding a library containing duplicate code that is not even being used (because of higher gas) feels wrong so I suggest we close this issue (by an admin) and everyone interested in using ds-math as library approach can checkout my forked implementation via https://github.com/JohannesMayerConda/ds-math. I've adapted the readme to reflect this. I think a separate repository using truffle makes sense anyway for being closer to openzeppelin SafeMath Library |
FYI most reputable tokens and protocols use OpenZeppelin's SafeMath, which is a library. The cost of gas is deemed acceptable for most projects out there. |
I was incorrect in the above comment about This removes the concern about excess runtime gas costs, but also the concern about deploytime gas costs. https://solidity.readthedocs.io/en/v0.5.1/contracts.html#libraries
|
It would be helpful if this were made into a library using Solidity's
library
.The text was updated successfully, but these errors were encountered: