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

Allow localhost enabling to be a parameter #2537

Closed
3 tasks done
Tracked by #3034
AdityaSripal opened this issue Oct 12, 2022 · 6 comments
Closed
3 tasks done
Tracked by #3034

Allow localhost enabling to be a parameter #2537

AdityaSripal opened this issue Oct 12, 2022 · 6 comments
Assignees

Comments

@AdityaSripal
Copy link
Member

AdityaSripal commented Oct 12, 2022

Perhaps we should add a parameter that allows chains to enable or disable localhost on their chain

This would then be checked in the localhost verification case to ensure it is enabled. Localhost verification should fail if the parameter is set to false

EDIT from @chatton

Additional tasks to be completed after the internal audit:

  • Use allowed clients list to dynamically enable/disable and client type.
  • Add Unauthorized status for light clients. This status should be returned if the client type is not in the allowed_list
  • Add light client status checks in ChanOpenInit and ConnOpenInit.
@AdityaSripal AdityaSripal added 09-localhost needs discussion Issues that need discussion before they can be worked on labels Oct 12, 2022
@colin-axner
Copy link
Contributor

colin-axner commented Oct 13, 2022

There already is a parameter in 02-client, closing

@colin-axner
Copy link
Contributor

One possible approach:

  • add a enable/disable field to client state
  • add msg to change this value (gov v1 msg, authority is only gov module)
  • update localhost client state upon execution
  • use client state field in Status

@damiannolan
Copy link
Member

damiannolan commented Feb 20, 2023

I think in order to do gov v1 message execution it will incur an API breaking change to the ibc core NewKeeper() function.

SDK modules which have migrated to use self managed params now take the additional arg to NewKeeper() functions: authority and store it as a field on the keeper. Where authority is the gov module account address.

See here and below where the additional authority arg is added

@colin-axner
Copy link
Contributor

colin-axner commented Feb 20, 2023

Could we preemptively get the API breaking change in v7 (without additional logic acting on the authority)? Or does it feel too late in the cycle for that?

@damiannolan
Copy link
Member

Sounds good to me. I think if its just changing the core NewKeeper function to accept an additional arg and store it as a field on the keeper then its totally fine

@crodriguezvega crodriguezvega removed the needs discussion Issues that need discussion before they can be worked on label Feb 27, 2023
@chatton
Copy link
Contributor

chatton commented Feb 28, 2023

closing this as #3196 has been merged into the feature branch.

@chatton chatton closed this as completed Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

5 participants