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

docs: Add new ERC-7579 Tutorial #589

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

docs: Add new ERC-7579 Tutorial #589

wants to merge 11 commits into from

Conversation

valle-xyz
Copy link
Collaborator

Replaces the old 7579 tutorial with a new one.

  • Compatible with the new permissionless.js 0.2.0 version and viem 2.20.0
  • Reduces boilerplate code
  • Adds three distinct functions to install, use and interact with a 7579 module
  • Accompanied by an example repository.

Copy link

github-actions bot commented Sep 16, 2024

Branch preview

✅ Deployed successfully in branch deployment:

https://tutorial_new7579--docs.review.5afe.dev

Copy link

github-actions bot commented Sep 16, 2024

Overall readability score: 42.52 (🟢 +0.03)

File Readability
7579-tutorial.mdx 73.6 (🟢 +3.53)
View detailed metrics

🟢 - Shows an increase in readability
🔴 - Shows a decrease in readability

File Readability FRE GF ARI CLI DCRS
7579-tutorial.mdx 73.6 57.37 8.57 8.9 10.08 6.45
  🟢 +3.53 🟢 +0.21 🟢 +0.37 🟢 +0.7 🟢 +0.53 🟢 +0.34

Averages:

  Readability FRE GF ARI CLI DCRS
Average 42.52 37.15 12.05 15.81 14.37 8.22
  🟢 +0.03 🟢 +0 🟢 +0 🟢 +0.01 🟢 +0 🟢 +0
View metric targets
Metric Range Ideal score
Flesch Reading Ease 100 (very easy read) to 0 (extremely difficult read) 60
Gunning Fog 6 (very easy read) to 17 (extremely difficult read) 8 or less
Auto. Read. Index 6 (very easy read) to 14 (extremely difficult read) 8 or less
Coleman Liau Index 6 (very easy read) to 17 (extremely difficult read) 8 or less
Dale-Chall Readability 4.9 (very easy read) to 9.9 (extremely difficult read) 6.9 or less

Copy link
Collaborator

@louis-md louis-md left a comment

Choose a reason for hiding this comment

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

Thanks @valle-xyz for the good work! The tutorials works as intended, so congrats on that!! 🙌 (And I know the kind of pain this means 😌). It's also much shorter, so kudos for that too, we'll be saving a lot of time to builders. 💪💪

I have noted a few improvement points:

  • We should probably explain to the user when/how is the safe deployed (and display e.g. the safe address, maybe with a link to the Safe Wallet UI?)
  • We could add numbers to the titles so it outlines the different steps we'll need to take (as it is for the title 1. Setup a Next.js application
  • At step "Interact with the 7579 module directly", there is a typo on the third bullet point: The transaction executes a call from your smart account call the module with the defined data.
  • Except for the console.logs, there is no user feedback after clicking the buttons

I also get a couple of type errors:

  • I get a type error as the object passed to toSafeSmartAccount should not have a chain property (at step Initialize the clients)
  • The useState are not typed. For example, the first useState should be:
const [safeAccount, setSafeAccount] = useState<ToSafeSmartAccountReturnType<EntryPointVersion> | null>(null)

Lastly, I have wider discussion points that I believe we should tackle with the rest of the team:

  • On the relevance of using OwnableExecutor, since Safes already have very similar functionality
  • Using privateKeyToAccount brings back the problem of having to manage private keys on the client-side
  • Should we bring back the layout from the other tutorial apps?

Anyway congrats again on your work on this! Let's continue the conversation on Slack 🚀

@germartinez germartinez changed the title Tutorial/new7579 docs: Add new ERC-7579 Tutorial Sep 19, 2024
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