Skip to content

Safe integration for mintHypercert() #37

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

Merged
merged 5 commits into from
Mar 11, 2025
Merged

Safe integration for mintHypercert() #37

merged 5 commits into from
Mar 11, 2025

Conversation

pheuberger
Copy link
Member

@pheuberger pheuberger commented Mar 11, 2025

This PR adds Safe support for minting a new Hypercert.

The way to switch the SDK over to propose a transaction to the Safe app is to add safeAddress to the overrides object.

You can see a Safe transaction created by the SDK here.

The Merkle trees had slight differences in their generic type. All
functions in the allowlist utils return the generic parameter [string,
bigint]
Since it's only used in one place we can inline.
One of the test cases experiences temporal coupling with the
getConnected() method. I tried to move the method call down to where it
is used and that broke the test.
Without this patch the caller is only able to mint a Hypercert with
their connected browser wallet.
Copy link

Coverage Report

Status Category Percentage Covered / Total
🟢 Lines 79.19% (🎯 78%) 1157 / 1461
🟢 Statements 79.19% (🎯 78%) 1157 / 1461
🟢 Functions 78.44% (🎯 77%) 91 / 116
🟢 Branches 85.25% (🎯 85%) 266 / 312
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
src/client.ts 71.47% 83.9% 55.55% 71.47% 81-83, 94-95, 105-106, 131-133, 177-209, 215-216, 220-221, 238-239, 251-257, 270-273, 288-298, 313-323, 329-336, 384-385, 428-429, 463-464, 513-520, 588-590, 665-666
src/safe/SafeTransactions.ts 97.22% 81.81% 100% 97.22% 25-26
src/types/client.ts 100% 100% 100% 100%
src/types/errors.ts 88.6% 90% 81.81% 88.6% 45, 84-87, 189-192
src/utils/allowlist.ts 57.5% 50% 75% 57.5% 7-20, 32-33, 38-39, 67
src/utils/overrides.ts 100% 100% 100% 100%
Generated in workflow #30 for commit d5f61ec by the Vitest Coverage Report Action

Copy link

@Jipperism Jipperism 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 to me. The interface is a bit less explicit then the one used in the marketplace which is fine, but we'd probably want to get that consistent at some point 👍

@pheuberger
Copy link
Member Author

Looks good to me. The interface is a bit less explicit then the one used in the marketplace which is fine, but we'd probably want to get that consistent at some point 👍

Yep, good point. We should harmonise this at some point. @bitbeckers and I decided on the quicker implementation to get this shipped sooner rather than later. Otherwise I would've spent some time to clean up the function, split it out probably using a strategy pattern to have a clean separation between the EOA and multisig cases. At any rate, mintHypercert() could use a do-over to fix the myriad of allowlist vs. non-allowlist code paths.

@pheuberger pheuberger merged commit 3930923 into main Mar 11, 2025
2 checks passed
@pheuberger pheuberger deleted the safe-integration branch March 11, 2025 18:27
@bitbeckers
Copy link
Contributor

🎉 This PR is included in version 2.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants