feat: adds a few more tips #1

devtooligan · 2021-12-14T00:34:58Z

This pr adds a few more tips to the README.md file.

Some notes/thoughts:

I spent a lot of time thinking about the top level "title" of each tip. On the one hand, I like showing the actual symbols >> but on the other hand it's kinda nice to write them out too Right shift -- my attempts at using both in the title did not come out well "Use Right shift (>>) instead of divide by two (/ 2)" (It looks even worse with >= or >)... Wdyt?
I saw you used two as a variable name which made sense in that case, but thought it might be more clear if standardize on the commonly used foo and bar
I could not quickly come up with a satisfactory explanation for these tips I added. And 10 minutes of googling or searching through t11's Twitter feed didn't come up with much either. I'm sure when I dive in and do the tests and look at the opcodes I will be able to come up with a great explanation. In the meantime if you want to add your explanation please do so, or just drop some links that would help
In general, I think the formatting / whitespace can be improved, need to think about it some more though
Is your ultimate goal for this something kind of light hearted with bits of "trivia" in it? I could see that, but I could also see it develop into something more serious like Solcurity standard but for optimization.
Ser what happened to the emoji commits? I was told there would be emoji commits

README.md

transmissions11 · 2021-12-15T03:46:21Z

nah imo descriptive var names would be helpful

on it

Would like to be more serious

🤝

wdyt we should do with formatting instead?

Well if it's me i'd like to go straight for the example, and maybe have detail explanation after w links?

Once we get a bunch of these we can think of the best ways to group and sort them. Can I ask you @transmissions11 when you think about optimizations does your mental model group them into any categories?

ha im tryna switch to conventional commits

🤝 classy

devtooligan · 2021-12-18T19:29:27Z

src/GolfCourse.sol

+// SPDX-License-Identifier: GPL-3.0-or-later
+pragma solidity ^0.8.6;
+
+contract GolfCourse {


for now I've put all these tests in one contract in one file, but I believe you would like to do one contract/file per example - please confirm

devtooligan · 2021-12-18T19:30:02Z

src/GolfCourse.sol

+pragma solidity ^0.8.6;
+
+contract GolfCourse {
+    function unoptimizedDivideByTwo() external pure returns (uint256 two) {


what do you think of the function naming? we could also consider a numbering system

devtooligan · 2021-12-18T19:36:47Z

README.md

+### 1.  When dividing by two, use `>> 1` instead of `/ 2` ###
+
+```solidity
+/// 🤦 Unoptimized (gas: 1401)


what do you think of showing the gas from the test in the comment here?

love it, tho may get hard to maintain after sol upgrades etc...

devtooligan · 2021-12-18T19:37:21Z

README.md


 The `SHR` opcode is 3 gas cheaper than `DIV` and also bypasses Solidity's division by 0 prevention overhead.
+  - [Gas Usage]()
+  - [Full Example](https://github.com/Rari-Capital/golf-course/blob/fc1882bacfec50787d9e9435d59fed4a9091fb21/src/GolfCourse.t.sol#L15)


I think you mentioned we should link to the test, which is what I've done here, but I was also thinking maybe it would be good to link to the example code?

yea i think we should do that instead of test tbh wdyt

we could even break convention and put the example code in the test file...

devtooligan · 2021-12-18T19:37:50Z

README.md

+
+### 3. Using `!=` is usually cheaper than `>` or `<` ###
+```solidity
+/// 🤦 Unoptimized (gas: 1423)


haha, I couldn't get this "tip" to work ... maybe it's a myth?

i think cuz of

function overhead in dapptools tests

we're using constants and not dynamic values so the compiler can optimize at compile time

Tried it with dynamic values for both sides of the equation. Also tried different values (like zero and non zero) No matter what I do, I was unable to repro gas savings. Taking this one off the list as well althought I'd be happy to be proved wrong on any of these.

Also, maybe because I'm using pragma 0.8.11???

Actually I figured it out. It only saves gas within a require()

devtooligan · 2021-12-18T19:39:29Z

README.md

 ```solidity
-// Unoptimized:
-uint256 two = 4 / 2;
+/// 🤦 Unoptimized (gas: 1535)


In spite of the increased variable overhead, the gas is still cheaper using the assembly. This kinda goes against our idea of putting in controls to ensure that every "optimization" is less gas than the unoptimized version. Wdyt? Perhaps we could have separate section for optimizing for variable overhead?

very possible its not a saving technique on its on, the savings do come in tho if u're accessing codesize via a library like Address.sol which has function call overhead associated. but yeah maybe not good for this guide then

removing it from tips, we'll put this on the back burner

devtooligan · 2022-02-13T22:17:07Z

Hey @transmissions11 I've spent a lot of time thinking about the formatting, and more importantly figuring out how we can get as accurate of gas usage numbers as possible.

There are so many things that could affect the gas usage including the ordering of the variables or functions and the Contract names. For that reason, I have decided to use two new contracts (optimized and unoptimized) for each tip.

If we had all the tips in one (or two) giant contracts and then we were to subsequently add a tip, it could potentially change all of the reported gas usages for all of the other functions/tests. This way still isn’t perfect, I've noticed in some cases, the gas usage reported by Dapp Tools can change (or not change) based on the name of the contract being different (even with everything else 100% the same). But I think it’s about as close as we can get for now with the tools we have.

Anyways, I think we've got some good tips to start out with. Review this and let me know what you think. Maybe we can merge this and do some sort of "soft opening" to the public soon to get comment and maybe some more submissions.

ZeroEkkusu · 2022-02-13T23:19:49Z

This may be a stupid idea, but, if contract names can cause problems and we want testing conditions to be as identical as possible, we could ~~make two dirs, src/unoptimized and src/optimized~~ have two files Unoptimized*.sol and Optimized*.sol for each optimization, where the contract names would be identical, everything would be identical, except for parts of code that were optimized.
Then, we could have a separate testing contract for every optimization, which would import the respective unoptimized and optimized versions:

import {DivideByTwo as UnoptimizedDivideByTwo} from "src/UnoptimizedDivideByTwo.sol"
import {DivideByTwo as OptimizedDivideByTwo} from "src/OptimizedDivideByTwo.sol"

I'm not on my computer now, so I'll try it tomorrow. This should make the testing conditions as identical as possible and could help with writing scripts for calculating gas savings.

devtooligan · 2022-02-14T05:18:32Z

@ZeroEkkusu Sounds like a good idea! I still think we should do the 2 contracts per tip for the reasons I stated, but if your idea works than we don't have to name the contracts OptimizedWhateverTip and UnoptimizedWhateverTip we could just name them WhateverTip

ZeroEkkusu · 2022-02-14T08:47:59Z

Okay, so I just tested those but don't get different gas usages because of contract names 🤔

Your solution with separate testing contracts works perfectly, though!

But, if you know names could cause a difference, we can name them the same, as I wrote above.

devtooligan · 2022-02-16T00:41:12Z

@ZeroEkkusu Try the reentrancy one. If you copy the optimized code to the unoptimized contract I see a diff in gas


Running 1 tests for src/tests/Optimized.t.sol:OptimizedTest
[PASS] testUseUintForReentrancy() (gas: 2125)

Running 1 tests for src/tests/Unoptimized.t.sol:UnoptimizedTest
[PASS] testUseUintForReentrancy() (gas: 2075)

As such I really like your idea of two separate folders with everything else the same including contract names. In fact I've been using your idea to test out some things in other projects and so far seems to work flawlessly.

When I have time I will update this pr to use that pattern

…names

ZeroEkkusu · 2022-02-16T13:51:09Z

@devtooligan 👍
I made a PR to improve the new tip.

I really like your idea of two separate folders

Drop the contracts. Just optimized, unoptimized. It's cleaner.

Improve the "Make functions `payable`" tip

…rse into some-tips-to-start

devtooligan · 2022-02-17T08:37:27Z

@ZeroEkkusu I just realized your idea of same named contracts in two different folders only works on Dapp Tools and not foundry :/ Oh well, I've been using Dapp Tools a lot on this one, esp the debugger.

ZeroEkkusu · 2022-02-17T08:56:27Z

@devtooligan Why is this a problem? Does naming testing contracts differently cause differences in reported gas savings? I'm using Foundry for the golf-course, btw. I know you can't name testing contracts the same, but haven't noticed any issues with your OptimizedSomething.t.sol -> OptimizedSomethingTest, UnoptimizedSomething.t.sol -> UnoptimizedSomethingTest approach. I named only the core contracts the same (Something.sol -> Something).

Will take a closer look at this today.

devtooligan · 2022-02-17T09:05:13Z

@ZeroEkkusu I upgraded the repo to use your suggestion it works great, just doesn't work w Foundry

I did find a discrepancy in gas based on contract name for the reentrancy tip. Try copy pasting the code from the unoptimizedRentrancy to the optimizedReentrancy contract so they both have the same code and run the test.

ZeroEkkusu · 2022-02-17T09:42:47Z

Dir structure:

src
	optimized
		Reentrancy.sol
	test
		OptimizedReentrancy.t.sol
		UnoptimizedReentrancy.t.sol
	unoptimized
		Reentrancy.sol

forge test normal contracts:

Running 1 test for OptimizedReentrancyTest.json:OptimizedReentrancyTest
[PASS] testUseUintForReentrancy() (gas: 2127)
Running 1 test for UnoptimizedReentrancyTest.json:UnoptimizedReentrancyTest
[PASS] testUseUintForReentrancy() (gas: 2360)

forge test but the code inside optimized/Reentrancy.sol and unoptimized/Reentrancy.sol is the same:

Running 1 test for OptimizedReentrancyTest.json:OptimizedReentrancyTest
[PASS] testUseUintForReentrancy() (gas: 2360)
Running 1 test for UnoptimizedReentrancyTest.json:UnoptimizedReentrancyTest
[PASS] testUseUintForReentrancy() (gas: 2360)

Maybe I'm missing something you're saying, @devtooligan.

Also, do you know that you have to force recompile when you change something in a core contract because there's currently a bug with Forge?

Here's the repo: https://github.com/ZeroEkkusu/golf-test

devtooligan · 2022-02-17T15:55:22Z

That's so weird, I'm seeing a difference with the differing contract names with both Dapp (top) and Forge

ZeroEkkusu · 2022-02-17T16:30:42Z

@devtooligan Could you clone the repo and try: https://github.com/ZeroEkkusu/golf-test

forge test --force

devtooligan · 2022-02-17T18:22:10Z

@ZeroEkkusu I will clone ur repo and do that just to further investigate the difference, but anyways I think we're going to drop the reentrancy tip because both DappTools and Foundry are erroneously reporting a large gas savings because neither handle gas refunds properly https://twitter.com/devtooligan/status/1494233623830138880?s=20&t=h7hwmiernm_Li41YClkEJw

So I wanna see if I can find another example of contract name causing a difference in reported gas usage as well.

✨ Add a few more tips

3afa909

devtooligan force-pushed the some-tips-to-start branch from 090f23d to 3afa909 Compare December 14, 2021 01:27

transmissions11 reviewed Dec 15, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

transmissions11 reviewed Dec 15, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

transmissions11 reviewed Dec 15, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

docs: update descriptions and formatting

0d796cb

devtooligan changed the title ~~✨ Add a few more tips~~ feat: Add a few more tips Dec 15, 2021

devtooligan changed the title ~~feat: Add a few more tips~~ feat: adds a few more tips Dec 15, 2021

devtooligan added 3 commits December 18, 2021 10:38

dapp init GolfCourse

5b1cad0

dapp install ds-test

244182e

feat: adds examples and tests

fc1882b

devtooligan commented Dec 18, 2021

View reviewed changes

docs: update readme.md with links and gas

889b45f

devtooligan commented Dec 18, 2021

View reviewed changes

feat: work on formatting and add a bunch of examples

fb1f401

devtooligan mentioned this pull request Feb 13, 2022

feat: function ordering #2

Open

fix: typo

48b3b6e

devtooligan added 3 commits February 15, 2022 16:52

feat: update comments

d33debe

feat: add RequireNeZero tip

b814df2

tweak: update gitignore

f45ed35

devtooligan and others added 6 commits February 15, 2022 16:59

fix: update directory structure to eliminate differences in contract …

70d07b7

…names

fix: update gas usage from new ctrct names

5606811

fix: update links in readme

52b02b0

feat: add payable functions tip

00d2a4f

fix: format code in markdown

c4cfffb

docs: add more info to "Make functions payable"

fc0707f

devtooligan and others added 4 commits February 16, 2022 14:23

Merge pull request #4 from ZeroEkkusu/work-1

d4dabe3

Improve the "Make functions `payable`" tip

fix: add tests

da9e4de

Merge branch 'some-tips-to-start' of github.com:Rari-Capital/golf-cou…

bfac3ce

…rse into some-tips-to-start

docs: update explanation

964c888

feat: new tip re: array +=

7b32928

feat: ArayPlus contracts

6bb2a4a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adds a few more tips #1

feat: adds a few more tips #1

devtooligan commented Dec 14, 2021

transmissions11 commented Dec 15, 2021 •

edited by devtooligan

Loading

devtooligan Dec 18, 2021

devtooligan Dec 18, 2021

devtooligan Dec 18, 2021 •

edited

Loading

transmissions11 Dec 21, 2021

devtooligan Dec 18, 2021

transmissions11 Dec 21, 2021

devtooligan Dec 22, 2021

devtooligan Dec 18, 2021

transmissions11 Dec 21, 2021

devtooligan Feb 13, 2022

devtooligan Feb 16, 2022

devtooligan Dec 18, 2021

transmissions11 Dec 21, 2021

devtooligan Feb 13, 2022

devtooligan commented Feb 13, 2022

ZeroEkkusu commented Feb 13, 2022 •

edited

Loading

devtooligan commented Feb 14, 2022

ZeroEkkusu commented Feb 14, 2022 •

edited

Loading

devtooligan commented Feb 16, 2022

ZeroEkkusu commented Feb 16, 2022

devtooligan commented Feb 17, 2022

ZeroEkkusu commented Feb 17, 2022 •

edited

Loading

devtooligan commented Feb 17, 2022

ZeroEkkusu commented Feb 17, 2022 •

edited

Loading

devtooligan commented Feb 17, 2022 •

edited

Loading

ZeroEkkusu commented Feb 17, 2022 •

edited

Loading

devtooligan commented Feb 17, 2022

feat: adds a few more tips #1

Are you sure you want to change the base?

feat: adds a few more tips #1

Conversation

devtooligan commented Dec 14, 2021

transmissions11 commented Dec 15, 2021 • edited by devtooligan Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devtooligan Dec 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devtooligan commented Feb 13, 2022

ZeroEkkusu commented Feb 13, 2022 • edited Loading

devtooligan commented Feb 14, 2022

ZeroEkkusu commented Feb 14, 2022 • edited Loading

devtooligan commented Feb 16, 2022

ZeroEkkusu commented Feb 16, 2022

devtooligan commented Feb 17, 2022

ZeroEkkusu commented Feb 17, 2022 • edited Loading

devtooligan commented Feb 17, 2022

ZeroEkkusu commented Feb 17, 2022 • edited Loading

devtooligan commented Feb 17, 2022 • edited Loading

ZeroEkkusu commented Feb 17, 2022 • edited Loading

devtooligan commented Feb 17, 2022

transmissions11 commented Dec 15, 2021 •

edited by devtooligan

Loading

devtooligan Dec 18, 2021 •

edited

Loading

ZeroEkkusu commented Feb 13, 2022 •

edited

Loading

ZeroEkkusu commented Feb 14, 2022 •

edited

Loading

ZeroEkkusu commented Feb 17, 2022 •

edited

Loading

ZeroEkkusu commented Feb 17, 2022 •

edited

Loading

devtooligan commented Feb 17, 2022 •

edited

Loading

ZeroEkkusu commented Feb 17, 2022 •

edited

Loading