Gas optimization for Ubiquity contracts

0xRizwan commented 7 months ago

Hi,

I have done gas optimization on contracts in development branch.

Link to check the gas optimization changes- https://github.com/ubiquity/ubiquity-dollar/compare/development...0xRizwan:ubiquity-dollar-Gas-optimization:development

Total gas saved from current implementation contracts- 13_38_283

cc- @pavlovcik and @molecula451

0x4007 commented 7 months ago

How long does something like this take to do?

Also @rndquu @gitcoindev rfc

rndquu commented 7 months ago

How long does something like this take to do?

Also @rndquu @gitcoindev rfc

I would put ~4 hours

molecula451 commented 7 months ago

i'm likely good with require() i would focus in gas optimizations in edges such as:

Minting
Deposit
Withdraw

Overall user experience but at the same time preserving what we architecturally like in terms of code definition.

Perhaps we would like to upgrade to Erroring Support or not

0xRizwan commented 7 months ago

How long does something like this take to do? Also @rndquu @gitcoindev rfc

I would put ~4 hours

Hey,

Thanks for the comment. This would definitely need ~6 hours. The tests also required to be changed. To be noted all tests are passed shared in above link.

0xRizwan commented 7 months ago

i'm likely good with require() i would focus in gas optimizations in edges such as:

Minting

Deposit

Withdraw

Overall user experience but at the same time preserving what we architecturally like in terms of code definition.

Perhaps we would like to upgrade to Erroring Support or not

custom errors are preferred over require/assert. This definately saves gas with use of custom errors. In case of require, i see the string message length was more than 32 bytes at some instances which means additional 1 slot is required in such case. Instead of using more than 1 slot for just return string message, i would still prefer custom errors. To be noted, custom errors has not been used fully in all contracts, at some places require is kept too.

I believe the contracts is being deployed on Ethereum mainnet. For user experience, i would request to see the possibility of L2 deployment. This definately increases number of users using the protocol due to low fees and the users frequently using functions like Minting, Deposit, Withdraw would incur very very low gas as compared to Ethereum.

For example: Taking my real life example. On average, I pay 4$ for Ethereum transaction and 0.25$ for Arbitrum transaction.

molecula451 commented 7 months ago

custom errors are preferred over require/assert. This definately saves gas with use of custom errors. In case of require, i see the string message length was more than 32 bytes at some instances which means additional 1 slot is required in such case. Instead of using more than 1 slot for just return string message, i would still prefer custom errors. To be noted, custom errors has not been used fully in all contracts, at some places require is kept too.

I'm sure custom errors are better implementation, consider this as a refactor, but changing to custom errors could be considered a refactor, instead of some gas optimization as there are require() working very good tho, feel free to start working on the issue

0xRizwan commented 7 months ago

/start

ubiquibot[bot] commented 7 months ago

! action has an uncaught error

molecula451 commented 7 months ago

/help

ubiquibot[bot] commented 7 months ago

Available Commands

Command	Description	Example
`/start`	Assign yourself to the issue.	/start
`/stop`	Unassign yourself from the issue.	/stop
`/help`	List all available commands.	/help
`/query`	Returns the user's wallet, access, and multiplier information.	/query @user
`/ask`	Ask a context aware question.	/ask is x or y the best approach?
`/multiplier`	Set the task payout multiplier for a specific contributor, and provide a reason for why.	/multiplier @user 0.5 "multiplier reason"
`/labels`	Set access control, for admins only.	/labels @user priority time price
`/authorize`	Approve a label change, for admins only.	/authorize
`/wallet`	Register your wallet address for payments.	/wallet ubq.eth

0x4007 commented 7 months ago

@0xRizwan Make sure to set your wallet address first. I think that's the unhandled error.

0xRizwan commented 7 months ago

/wallet 0x9Ea3efa3F1145A46c4eEc34B5a995De570b8050b

ubiquibot[bot] commented 7 months ago

+ Successfully registered wallet address

0xRizwan commented 7 months ago

/start

ubiquibot[bot] commented 7 months ago




Deadline Wed, Feb 14, 5:01 AM UTC

Registered Wallet
0x9Ea3efa3F1145A46c4eEc34B5a995De570b8050b

Tips:

Use /wallet 0x0000...0000 if you want to update your registered payment wallet address.
Be sure to open a draft pull request as soon as possible to communicate updates on your progress.
Be sure to provide timely updates to us when requested, or you will be automatically unassigned from the task.

molecula451 commented 7 months ago

I believe the contracts is being deployed on Ethereum mainnet. For user experience, i would request to see the possibility of L2 deployment. This definately increases number of users using the protocol due to low fees and the users frequently using functions like Minting, Deposit, Withdraw would incur very very low gas as compared to Ethereum.

Hey We are going Mainnet, so consider that do not take in count L2s

molecula451 commented 7 months ago

custom erros can also be inside require() that's why the change could be considered a refactor

0xRizwan commented 7 months ago

Please check PR

Overall gas change from current implementation: 6.6 million which is ~2.3 % change.

Feel free to connect for any clarifications.

ubiquibot[bot] commented 7 months ago

# These linked pull requests are closed:  <a href="https://github.com/ubiquity/ubiquity-dollar/pull/896">#896</a>

ubiquibot[bot] commented 7 months ago

! action has an uncaught error

ubiquibot[bot] commented 7 months ago

! action has an uncaught error

0x4007 commented 7 months ago

I'll try salvaging the permits from the logs here.

ubiquibot[bot] commented 7 months ago

+ Evaluating results. Please wait...

0x4007 commented 7 months ago

[ 18.3 WXDAI ]

@pavlovcik

Contributions Overview

View	Contribution	Count	Reward
Issue	Comment	3	3.7
Review	Comment	3	14.6

Conversation Incentives

Comment	Formatting	Relevance	Reward
How long does something like this take to do? Also @rndquu @git...	1.3	0.49	1.3
@0xRizwan Make sure to set your wallet address first. I think th...	1.6	0.44	1.6
I'll try debugging the permit flow here. ...	0.8	0.45	0.8
> `require` used with return string message adds more readabilit...	4.4 a: count: 1 score: "1" words: 1 code: count: 1 score: "1" words: 1	0.75	4.4
> i think we could perhaps compensate the user for these comment...	4.4	0.57	4.4
Yes unfortunately the permit flow is a coin toss right now. I th...	5.8 li: count: 1 score: "1" words: 8	0.56	5.8

[ 29.5 WXDAI ]

@molecula451

Contributions Overview

View	Contribution	Count	Reward
Issue	Comment	4	18.3
Review	Comment	3	11.2

Conversation Incentives

Comment	Formatting	Relevance	Reward
i'm likely good with `require()` i would focus in gas optimizati...	8.8 li: count: 3 score: "3" words: 3 code: count: 1 score: "1" words: 1	0.7	8.8
> custom errors are preferred over require/assert. This definate...	5.4 code: count: 1 score: "1" words: 1	0.73	5.4
> I believe the contracts is being deployed on Ethereum mainnet....	1.4	0.45	1.4
custom erros can also be inside `require()` that's why the chang...	2.7 code: count: 1 score: "1" words: 1	0.51	2.7
Please fix the build actions as some are not passing...	1	0.31	1
Yeah i think the PR won't make it either as the such refactors s...	7.7	0.68	7.7
it looks like it took action on the PR closing but not on the is...	2.5	0.59	2.5

[ 21.2 WXDAI ]

@gitcoindev

Contributions Overview

View	Contribution	Count	Reward
Review	Comment	1	21.2

Conversation Incentives

Comment	Formatting	Relevance	Reward
Hi everyone, I am back. I analysed the changes. Now I see where ...	21.2 li: count: 3 score: "3" words: 43	0.61	21.2

[ 89.8 WXDAI ]

@0xRizwan

Contributions Overview

View	Contribution	Count	Reward
Issue	Specification	1	7.2
Issue	Comment	3	44.6
Review	Comment	2	38

Conversation Incentives

Comment	Formatting	Rele 0x4007 commented 7 months ago Looks like the qualitative analysis is no longer applying its multiplier. I see on gitcoindev's comment that it clearly is not doing anything: raw score of 21.2 with a relevance of 0.61 should yield 12.932 Looks like I already filed the issue here https://github.com/ubiquibot/comment-incentives/issues/8 0xRizwan commented 7 months ago @pavlovcik @molecula451 and team, Thank you. © Githubissues. Githubissues is a development platform for aggregating issues.

ubiquity / ubiquity-dollar