Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix twin inefficiencies #656

Merged
merged 15 commits into from
Jul 7, 2023
Merged

Fix twin inefficiencies #656

merged 15 commits into from
Jul 7, 2023

Conversation

anajuliabit
Copy link
Contributor

@anajuliabit anajuliabit commented May 29, 2023 •
edited
Loading

Fix #630

@anajuliabit anajuliabit changed the title Added functionality to validate availability of twin ranges for selle… Fix twin inefficiencies May 29, 2023
@anajuliabit anajuliabit marked this pull request as ready for review May 31, 2023 00:54
@anajuliabit anajuliabit self-assigned this May 31, 2023
@anajuliabit anajuliabit requested review from mischat and zajck May 31, 2023 00:55
zajck
zajck reviewed Jun 2, 2023
View reviewed changes
Copy link
Member

@zajck zajck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I just added a question about potential performance enhancement.

Also, I think it would be nice to have a test that sort of shows that scenario 1. from #630 is possible now (i.e. twin can be reused).

contracts/protocol/facets/ExchangeHandlerFacet.sol Outdated Show resolved Hide resolved
contracts/protocol/facets/ExchangeHandlerFacet.sol Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jun 6, 2023 •
edited
Loading

Coverage Status

coverage: 99.706% (+0.002%) from 99.704% when pulling 055a44d on createTwin_inefficiencies into 7cf35c4 on main.

@anajuliabit anajuliabit requested a review from zajck June 7, 2023 09:56
zajck
zajck requested changes Jun 8, 2023
View reviewed changes
contracts/protocol/bases/TwinBase.sol Show resolved Hide resolved
contracts/protocol/facets/ExchangeHandlerFacet.sol Show resolved Hide resolved
contracts/protocol/facets/TwinHandlerFacet.sol Outdated Show resolved Hide resolved
contracts/protocol/facets/TwinHandlerFacet.sol Outdated Show resolved Hide resolved
contracts/protocol/facets/ExchangeHandlerFacet.sol Outdated Show resolved Hide resolved
@anajuliabit anajuliabit requested a review from zajck June 8, 2023 19:16
@anajuliabit anajuliabit requested a review from zajck June 9, 2023 12:49
zajck
zajck previously approved these changes Jun 9, 2023
View reviewed changes
Copy link
Member

@zajck zajck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zajck zajck mentioned this pull request Jun 16, 2023
Merged
@anajuliabit anajuliabit mentioned this pull request Jul 6, 2023
Merged
@mischat
Copy link
Member

mischat commented Jul 6, 2023

@anajuliabit it looks like we need to fix the conflicts here ^^

@anajuliabit anajuliabit mentioned this pull request Jul 6, 2023
Merged
@anajuliabit
Copy link
Contributor Author

anajuliabit commented Jul 6, 2023 •
edited
Loading

@mischat @zajck , I fixed the conflicts but looks like I messed up the storage slot calculation, as there were 4 failing tests on ExchangeHandlerTest.js. I'm calling it a day for now, but I'll make sure to fix them first thing tomorrow morning

@mischat
Copy link
Member

mischat commented Jul 6, 2023

@anajuliabit just to say that the tests are still failing here...

@anajuliabit
Copy link
Contributor Author

@mischat #656 (comment)
yes I know I left a note here :)

@zajck
Copy link
Member

zajck commented Jul 7, 2023

@anajuliabit I pushed the fix for failing tests. Slot calculation was ok, there were just some ethersv6 related breaking changes.

mischat
mischat approved these changes Jul 7, 2023
View reviewed changes
Copy link
Member

@mischat mischat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ... nice to pull out that for loop

contracts/protocol/facets/TwinHandlerFacet.sol
twinRanges.pop();

// Delete rangeIdByTwin mapping
delete lookups.rangeIdByTwin[_twinId];
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is fun :)

@mischat mischat requested a review from zajck July 7, 2023 10:03
@zajck zajck merged commit d358797 into main Jul 7, 2023
9 checks passed
@zajck zajck deleted the createTwin_inefficiencies branch July 7, 2023 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mischat mischat mischat approved these changes

@zajck zajck zajck approved these changes

Assignees

@anajuliabit anajuliabit

Labels
v2.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

createTwin inefficiencies
4 participants
@anajuliabit @coveralls @mischat @zajck