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

Refine minor improvements #239

Merged
merged 9 commits into from
Oct 3, 2023
Merged

Conversation

jaybxyz
Copy link
Contributor

@jaybxyz jaybxyz commented Sep 22, 2023

Description

This PR refines minor improvements:

  • Remove pending TODO comment of how to set authority.
    • NewBaseKeeper it is set with governance module account in app.go)
  • Remove duplicate validation in AddAccountToFundModuleSet as stateless check is already done during CheckTx.
  • Fix ValidateBasic for MsgAddAccountToFundModuleSet. It was validating the same address twice.
  • Use types.ModuleName throughout the modules instead of constant value. Notice that the ratelimit module was having transfermiddleware value in its tx.go.

Please, feel free to let me know if there are other minor improvements that can be made in this PR. I am happy to contribute.

@jaybxyz jaybxyz marked this pull request as ready for review September 22, 2023 08:53
@jaybxyz
Copy link
Contributor Author

jaybxyz commented Sep 25, 2023

@GNaD13 @vuong177 Interchain Tests workflow action fails due to the following error. Do you have any idea how to resolve this?

Error: buildx failed with: ERROR: failed to solve: failed to push ghcr.io/notional-labs/centauri-ictest:latest: unexpected status from POST request to https://ghcr.io/v2/notional-labs/centauri-ictest/blobs/uploads/: 403 Forbidden

@vuong177
Copy link
Contributor

@anhductn2001 could you check the ghrc problem? I think we need to add @jaybxyz to centauri repo ghrc.

@anhductn2001
Copy link
Contributor

Ok, Let me see! Seem like login ghcr.io step was success.

@anhductn2001
Copy link
Contributor

I think the problem maybe come from your source repo @jaybxyz. We can temporarily skip this test!

@jaybxyz
Copy link
Contributor Author

jaybxyz commented Sep 25, 2023

I think it is known issue. It seems like a lot of devs are experiencing the same / similar issue. Let me try some of the solutions and see if it works.

  1. Some suggests to create a new Docker image (UPDATE: seems like it is only a temporary solution)
  2. (Not working) Some suggests to add an explicit permissions to the .yml file
    permissions:
      contents: read
      packages: write
  1. (Not working) Replace github.repository_owner to github.actor

References

@jaybxyz jaybxyz self-assigned this Sep 25, 2023
@jaybxyz
Copy link
Contributor Author

jaybxyz commented Sep 25, 2023

@vuong177 @anhductn2001 Can you reference this https://github.com/orgs/community/discussions/26274#discussioncomment-3251137 and try to see if it works? Otherwise, i don't know how to resolve this strange issue.

So to fix this, head over to $yourOrganization → Packages → $yourPackage → Package settings (to the right / bottom)

And configure “Manage Actions access” section to allow the git repository in question write permissions on this package/docker repository

@anhductn2001
Copy link
Contributor

I'm pretty sure we have write permission set for it. This is an issue related to Github Action that I recently encountered when creating a PR to a repository from our Notional-labs fork source.

@jaybxyz
Copy link
Contributor Author

jaybxyz commented Sep 26, 2023

@anhductn2001 Are you trying something to resolve this issue in #243 ? I've tried to add an explicit permission but it didn't resolve. See 5076078.

@anhductn2001
Copy link
Contributor

Yes, @jaybxyz. I just add permission in another place. Can you merge master to your branch?

@jaybxyz
Copy link
Contributor Author

jaybxyz commented Sep 26, 2023

@anhductn2001 It doesn't seem to resolve the issue. What else can we try? Some suggests to create a new Docker image ghcr.io/notional-labs/centauri-ictest:latest.

@anhductn2001
Copy link
Contributor

Perhaps this is a Github issue. I think first let's review and merge it to master. Because build and push images on master and all other branches are working normally.

@jaybxyz
Copy link
Contributor Author

jaybxyz commented Sep 27, 2023

@anhductn2001 What is the protocol regarding the review process? Am I allowed to merge after at least one reviewer has completed their review?

@anhductn2001
Copy link
Contributor

I think we should need 2 approvals. Let's wait @vuong177 review it!

@anhductn2001 anhductn2001 requested review from vuong177 and removed request for vuong177 September 28, 2023 09:31
@@ -70,10 +70,6 @@ func (ms msgServer) FundModuleAccount(goCtx context.Context, req *types.MsgFundM

func (ms msgServer) AddAccountToFundModuleSet(goCtx context.Context, req *types.MsgAddAccountToFundModuleSet) (*types.MsgAddAccountToFundModuleSetResponse, error) {
ctx := sdk.UnwrapSDKContext(goCtx)
err := req.ValidateBasic()
if err != nil {
return nil, errorsmod.Wrapf(types.ErrValidationMsg, "invalid req msg %v - err %v", req, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

good catch! we don't duplicate SDK process!

@vuong177 vuong177 merged commit 2df0581 into ComposableFi:master Oct 3, 2023
11 of 12 checks passed
tungleanh0902 pushed a commit to notional-labs/composable-cosmos that referenced this pull request Mar 7, 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.

4 participants