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.

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

Chaining compatibility #6

Merged
merged 2 commits into from
Sep 10, 2024
Merged

Chaining compatibility #6

merged 2 commits into from
Sep 10, 2024

Conversation

spengrah
Copy link
Member

@spengrah spengrah commented Sep 6, 2024

This PR makes the module compatible with module chaining by replacing the push approach to burning the hat (IHats.setHatWearerStatus) with a pull approach (IHats.checkHatWearerStatus). The former will not work when the module is chained with other modules since, while the latter will.

Additionally, I've bumped the libs and am using HatsModuleFactory v0.7.0 in the tests, which requires an additional salt nonce argument.

Copy link

github-actions bot commented Sep 6, 2024

Changes to gas cost

Generated at commit: aa37e96294a41c3cfc3ce92ab9041b62b0b119af, compared to commit: 07b05944a31c26cbf9c6977099d0e5a140e7c773

🧾 Summary (10% most significant diffs)

Contract Method Avg (+/-) %
AgreementEligibility forgive
revoke
+3,057 ❌
+1,482 ❌
+10.00%
+2.05%

Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
AgreementEligibility 0 (0) forgive
getWearerStatus
revoke
21,602 (0)
678 (0)
21,600 (0)
0.00%
0.00%
0.00%
33,639 (+3,057)
1,712 (-29)
73,888 (+1,482)
+10.00%
-1.67%
+2.05%
33,639 (+3,057)
982 (0)
84,346 (+1,778)
+10.00%
0.00%
+2.15%
45,677 (+6,114)
7,108 (0)
84,346 (+1,778)
+15.45%
0.00%
+2.15%
2 (0)
71 (+6)
6 (0)
HatsModuleFactory 764,578 (+27,626) createHatsModule 154,237 (+470) +0.31% 186,756 (+470) +0.25% 186,756 (+470) +0.25% 219,276 (+470) +0.21% 70 (0)

@spengrah spengrah requested a review from gershido September 6, 2024 15:47
Copy link
Contributor

@gershido gershido left a comment

Choose a reason for hiding this comment

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

LGTM

@spengrah spengrah merged commit 4f5066a into main Sep 10, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants