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

refactor: ensure the UX for new contracts is consistent with the parachains. #232

Merged
merged 7 commits into from
Jul 8, 2024

Conversation

AlexD10S
Copy link
Collaborator

Ensure the UX for new contracts is consistent with the parachains. Remove the --path flag for contracts to match the behavior of parachains. Restructure the user guidance for generating a new contract to follow the same order as parachains: 1. template to use 2. name (path) of the project.

Copy link

codecov bot commented Jun 30, 2024

Codecov Report

Attention: Patch coverage is 78.12500% with 7 lines in your changes missing coverage. Please review.

Project coverage is 70.84%. Comparing base (ac0ce23) to head (66729b0).

@@            Coverage Diff             @@
##             main     #232      +/-   ##
==========================================
+ Coverage   70.78%   70.84%   +0.05%     
==========================================
  Files          47       47              
  Lines        7445     7443       -2     
  Branches     7445     7443       -2     
==========================================
+ Hits         5270     5273       +3     
+ Misses       1289     1284       -5     
  Partials      886      886              
Files Coverage Δ
crates/pop-common/src/helpers.rs 81.57% <100.00%> (+9.57%) ⬆️
crates/pop-cli/src/commands/new/contract.rs 48.99% <63.15%> (-1.01%) ⬇️

... and 1 file with indirect coverage changes

Copy link
Contributor

@brunopgalvao brunopgalvao left a comment

Choose a reason for hiding this comment

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

When I run cargo run new contract and select ERC20, for some reason it hangs on:

┌   Pop CLI : Generating "duck" using Erc20!
│
◓  Generating contract...   

@AlexD10S
Copy link
Collaborator Author

AlexD10S commented Jul 2, 2024

When I run cargo run new contract and select ERC20, for some reason it hangs on:

┌   Pop CLI : Generating "duck" using Erc20!
│
◓  Generating contract...   

Hmmm It works for me. What is doing is cloning the ERC20 from https://github.com/use-ink/ink-examples/tree/main/erc20
Is possible your internet connection is poor?

@evilrobot-01
Copy link
Contributor

Do we need to clone it? We have logic for downloading source code archives from GitHub, which is much faster, especially as we don't need the commit history for the created contract?

Perhaps the sourcing module should be moved from parachains to common?

Copy link
Contributor

@evilrobot-01 evilrobot-01 left a comment

Choose a reason for hiding this comment

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

A few improvements still required.

crates/pop-cli/src/commands/new/contract.rs Show resolved Hide resolved
crates/pop-common/src/helpers.rs Outdated Show resolved Hide resolved
crates/pop-common/src/helpers.rs Outdated Show resolved Hide resolved
crates/pop-common/src/lib.rs Show resolved Hide resolved
@AlexD10S
Copy link
Collaborator Author

AlexD10S commented Jul 3, 2024

Do we need to clone it? We have logic for downloading source code archives from GitHub, which is much faster, especially as we don't need the commit history for the created contract?

Perhaps the sourcing module should be moved from parachains to common?

This is out of scope for this PR.
I agree that it's an improvement to make. I'm working on it in a different PR: #239.

@AlexD10S
Copy link
Collaborator Author

AlexD10S commented Jul 4, 2024

When I run cargo run new contract and select ERC20, for some reason it hangs on:

┌   Pop CLI : Generating "duck" using Erc20!
│
◓  Generating contract...   

Can you try this PR: #239 to see if you still experience this issue?

@brunopgalvao
Copy link
Contributor

brunopgalvao commented Jul 5, 2024

Same issue for me. This is also happening on main for me as well.

@brunopgalvao
Copy link
Contributor

When I run cargo run new contract and select ERC20, for some reason it hangs on:

┌   Pop CLI : Generating "duck" using Erc20!
│
◓  Generating contract...   

This is now working for me. I believe it was an internet issue. I will continue to monitor this in case it happens again and discover the source of the issue.

@AlexD10S
Copy link
Collaborator Author

AlexD10S commented Jul 8, 2024

As Frank suggested in a comment above, downloading source code archives from GitHub is much faster than cloning the repository. I have initiated this improvement in another PR: : #239 However, it will require more time for a proper refactor of sourcing logic.

If you encounter the error again, please debug to gather more information to replicate it accurately.

@evilrobot-01
Copy link
Contributor

Note that this is missing the audit warning that we have in the parachains ux, which should be added as a separate PR.

@AlexD10S AlexD10S merged commit e4dc4ed into main Jul 8, 2024
15 checks passed
@AlexD10S AlexD10S deleted the alex/refactor-ux branch July 8, 2024 10:22
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.

3 participants