search

ubiquity / ubiquity-dollar

Ubiquity Dollar (uAD) smart contracts and user interface.
https://uad.ubq.fi
Apache License 2.0
31 stars 82 forks source link

Gas optimization #896

Closed 0xRizwan closed 4 months ago

0xRizwan commented 4 months ago

Resolves #895

molecula451 commented 4 months ago

Please fix the build actions as some are not passing

0xRizwan commented 4 months ago

Hi team,

This is regarding to error pertaining to test coverage. error is due to mismatch of coverage and for this reason, check is not passing.

Development branch coverage: 79.0
PR branch coverage: 78.9

I have analysed the issue but not sure where 0.1 tests are decreased. I am quite sure, while testing none of the tests were deleted which can be checked in commits.

On overall gas saving,

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

I haven't checked each function gas saving which is bit time consuming. However i see the goal to gas saving is acheived and i think 6.6 million is pretty good number.

I would request the team to review the changes and revert for any clarifications.

0xRizwan commented 4 months ago

Look like you need to format your code. Also by "custom error codes" I thought you meant like how Gnosis Safe does it, like "GS018" and then the user goes to their documentation to see the full description of the error which makes a ton of sense to me.

Imo, This affects user experience in searching error code meaning. require used with return string message adds more readability and easy to understand for a normal user.

0x4007 commented 4 months ago

require used with return string message adds more readability and easy to understand for a normal user.

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. ^1^

Hard disagree if we need to explain within 32 characters vs unlimited character length hosted on our website with examples and diagrams if necessary.

molecula451 commented 4 months ago

Yeah i think the PR won't make it either as the such refactors should be considered with prior conversation among core team members (best approach, just like https://github.com/ubiquity/ubiquity-dollar/issues/889), but the intention contribution worth it and give a bit of insight, i think we could perhaps compensate the user for these comment and the PR effort a bit and mark this as not planned and close it

my best thoughts

gitcoindev commented 4 months ago

Hi everyone, I am back. I analysed the changes. Now I see where the pull request comes from the optimization boils down to three kinds of refactoring:

Thank you as well for posting the detailed results. Overall it seems that most of the gas savings are detected in testUUPS_* tests , and for the diamond part the gas savings are negligible. I agree with @molecula451 that perhaps we could think of compensation for the effort , but I would not merge this as is. At least now, perhaps consider this and other gas saving optimizations in a later stage of the project and leave the current state on a separate branch as @rndquu suggested. I found a quite good article on possible optimizations that also gives other options that could be applied: https://0xmacro.com/blog/solidity-gas-optimizations-cheat-sheet/

0x4007 commented 4 months ago

i think we could perhaps compensate the user for these comment and the PR effort a bit and mark this as not planned and close it

If the bot decides to work as designed, then we should be able to unassign 0xRizwan and close the issue as complete. This should generate payment permits for all on topic comments, as well as generate a larger reward for authoring the issue specification.

molecula451 commented 4 months ago

it looks like it took action on the PR closing but not on the issue https://github.com/ubiquity/ubiquity-dollar/issues/895#issuecomment-1944481633

0x4007 commented 4 months ago

Yes unfortunately the permit flow is a coin toss right now. I think it works about half the time. Check https://github.com/ubiquibot/comment-incentives/actions for the status on these runs

The most prominent problem is specified here: