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

feat(templates): add fee abstraction into the scaffold chain template #4181

Closed
wants to merge 26 commits into from

Conversation

trungnt1811
Copy link

@trungnt1811 trungnt1811 commented Jun 7, 2024

Closes: #4171

@trungnt1811 trungnt1811 marked this pull request as draft June 7, 2024 05:07
@trungnt1811 trungnt1811 changed the title Add fee abstraction into the default template feat: Add fee abstraction into the default template Jun 7, 2024
@trungnt1811 trungnt1811 changed the title feat: Add fee abstraction into the default template feat: Add fee abstraction into the scaffold chain template Jun 16, 2024
@trungnt1811 trungnt1811 marked this pull request as ready for review June 27, 2024 08:33
@trungnt1811 trungnt1811 changed the title feat: Add fee abstraction into the scaffold chain template feat(templates): add fee abstraction into the scaffold chain template Jun 27, 2024
@trungnt1811
Copy link
Author

Hi @julienrbrt, could you take a look at this PR? Thanks

@trungnt1811
Copy link
Author

@faddat the chain template including feeabs generated from the CLI is working well. Please take a look at this, sir

@github-actions github-actions bot added component:cmd type:services Service-related issues. labels Jul 12, 2024
@trungnt1811
Copy link
Author

As discussed above, I have created a new template named "files-feeabs" that includes the fee abstraction module. The chain can be scaffolded using the --fee-abstraction flag. For example: ignite scaffold chain example --fee-abstraction. @julienrbrt please help me review this PR, sir

@trungnt1811 trungnt1811 marked this pull request as ready for review July 12, 2024 07:58
@julienrbrt
Copy link
Member

As discussed above, I have created a new template named "files-feeabs" that includes the fee abstraction module. The chain can be scaffolded using the --fee-abstraction flag. For example: ignite scaffold chain example --fee-abstraction. @julienrbrt please help me review this PR, sir

This is a good temporary solution, thank you! When we have the module registry ready, we can make this fallback to the app.

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

You actually only need to copy the files that are different from files/ instead of having everything. Other than that lgtm.

@trungnt1811
Copy link
Author

You actually only need to copy the files that are different from files/ instead of having everything. Other than that lgtm.

Thank you so much. I have removed the redundant template. Please take a look at this, sir.

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

thanks! I think it is good for now.
In a follow-up we should refactor this to improve the UX and have a --external-modules flag instead of a fee abstraction modules. As currently this module is mutually exclusive with the other, while it would be best if it was additive.

We can accept this on main, but we'll need to fix the UX before backporting on v28

What do you think @Pantani / @toschdev ?

@julienrbrt julienrbrt added the skip-changelog Don't check changelog for new entries label Jul 18, 2024
@Pantani
Copy link
Collaborator

Pantani commented Aug 21, 2024

thanks! I think it is good for now. In a follow-up we should refactor this to improve the UX and have a --external-modules flag instead of a fee abstraction modules. As currently this module is mutually exclusive with the other, while it would be best if it was additive.

We can accept this on main, but we'll need to fix the UX before backporting on v28

What do you think @Pantani / @toschdev ?

IMHO, we should only add official modules. This way, we have more modules to maintain and add support; the fee abstraction module still needs some updates to be in the last stack. Let's add this integration as an app/extension

@julienrbrt
Copy link
Member

thanks! I think it is good for now. In a follow-up we should refactor this to improve the UX and have a --external-modules flag instead of a fee abstraction modules. As currently this module is mutually exclusive with the other, while it would be best if it was additive.

We can accept this on main, but we'll need to fix the UX before backporting on v28

What do you think @Pantani / @toschdev ?

IMHO, we should only add official modules. This way, we have more modules to maintain and add support; the fee abstraction module still needs some updates to be in the last stack. Let's add this integration as an app/extension

100% agree, however to I wouldn't create an app per custom module but I more general app. This is someone I will be building. So to ship this faster maybe we should include it as it in v28 and improve it for v29 (like we will do for the consumer app).

@julienrbrt
Copy link
Member

Hi, we just discussed this in our team call, we like this feature and want to integrate it right now.
However, we won't integrate it as it stands (directly into ignite cli), as we have a policy to only integrate cosmos sdk / ibc modules. Other scaffolding capabilities should be implemented as Ignite App as @Pantani mentioned.

@Pantani is integrating this module as an Ignite App using hooks on the scaffold chain commands as we speak.
I'll be closing this in the meantime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:ci CI/CD workflow and automated jobs. component:cmd component:configs component:packages component:templates skip-changelog Don't check changelog for new entries type:services Service-related issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

INIT: Fee Abstraction
4 participants