1

this is my first post on the Ethereum stack exchange. I've been a software engineer for the past 6 or so years and have recently started going in the direction of blockchain development. As a developer I strongly feel that the best way to learn is hands on, and so I decided doing some solidity projects. I really prefer these projects to help the blockchain community because I am very passionate about Web3.0 My first project is a test-net ETH pool (the idea was born when I noticed a lot of faucets have dried up or are dripping very small amounts of test-net ETH, making it harder to pay gas costs while testing projects). I uploaded it as an S3 distribution on AWS to this domain https://testnet-ethpool.xyz/ and anyone with some excess test-net ETH (on any test-net) can donate some of it (withdrawal is limited for 1 ETH per cycle and a cycle should be about 1 week)

The repo for the contract is here

As I mentioned previously I am very new to the world of blockchain development and might still have many bad habits originating from my years as a normal web developer, so I will be extremely happy to get some tips and pointers about what I could have done better. If this contract is inefficient gas-wise or if it's vulnerable to attacks I would be extremely grateful to know as well.

Thank you very much and hope to be a productive member on this community, Have a great new year!

1 Answer 1

0

I've read your contract, here's some feedback:

  1. Creating an address array and iterating over it is considered an anti pattern in Solidity. This approach will not work because this address array can grow very large, and then when you reset the epoch not only will it cost a lot of gas, but it might also reach the block gas limit, and then the function will always revert and you will not be able to reset the epoch.

    • One solution for that is instead of saving address[] public addrList, make it a double array, where the first index is the epoch ID and the second index is the address (same as it is now). Then when you reset the epoch, you insert a new empty address[] public addrList to the double array. And always consider the last element of the double array to be the current addrList. This way you're effectively disregarding the previous values without having to iterate over them.

    • Another probably nicer solution is to save for each address when was the last time it claimed, and then when it tries to claim again, only let it claim if resetInterval time has passed. This way you also don't need to manually reset the epoch.

  2. When using uints which are smaller than uint256, it might actually cost more gas sometimes because Solidity has to cast them to uint256. You can read up about variable packing.

Good luck 🙂

5
  • 1
    Thank you so much for your response, this helps a lot. I was wondering if having uncleaned up memory will cost more than cleaning it up. Optimally I would have just deleted the entire mapping and remade it, but I understand this isn't possible. So considering the double array / hash map might grow indefinitely it would still be more cost efficient than iterating and deleting right? Also I didn't know about the uint variables being cast to 256, again thank you, I will look up on variable packing. I wanted to upvote your response but I don't have enough reputation yet to do that. Commented Jan 2, 2022 at 17:34
  • You're welcome 🙂 Cleaning the memory will refund you some gas, but it is capped relative to the gas consumed ( github.com/wolflo/evm-opcodes/blob/main/gas.md#a0-3-gas-refunds ), so you'll still spend gas when iterating over the array and deleting it, and therefore you'll spend a lot of gas and might reach the block gas limit at some point. Because of this reason, indeed the double array will not cost more than iterating - deleting the array still costs you gas, and in the double array method you don't even need to delete the array. Commented Jan 3, 2022 at 1:53
  • Thank you again! I have implemented the fixes you suggested and updated the repo (should I edit my original post?) There has been a slight change in functionality. currently a user can't wait to the end of an epoch and withdraw twice in a short period, but has to wait the entire interval. Additionally each user now has their own personal epoch. I will await further suggestions before redeploying the code to all networks and changing the code on the distribution. I really appreciate your help :) Commented Jan 3, 2022 at 13:56
  • Did you push the changes to the repo? It looks like there's no new commit there Commented Jan 3, 2022 at 14:25
  • Sorry my bad, the terminal hanged and I just alt + tab not noticing. Commit went through now Commented Jan 3, 2022 at 23:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.