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

Implement ICA as middleware on controller side #313

Closed
3 tasks
colin-axner opened this issue Aug 5, 2021 · 6 comments
Closed
3 tasks

Implement ICA as middleware on controller side #313

colin-axner opened this issue Aug 5, 2021 · 6 comments

Comments

@colin-axner
Copy link
Contributor

Summary

The controller side of the interchain accounts module is middleware which can be connected to a helper module which might construct the messages to be sent to the counterparty chain


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@colin-axner colin-axner added this to the Interchain Accounts milestone Aug 5, 2021
@colin-axner
Copy link
Contributor Author

Starting to look into this. I'm thinking about creating a router for ICA. I'd like to route to different authentication (helper) modules based on the owner address. The alternative solution is initializing an interchain account module per authentication module

So lets say we have 3 authentication modules:

  • gov
  • experimental auth 1
  • experimental auth 2 (imagine these are zksnarks or some new cryptography stuff)

So in the current design, one might initialize a new interchain accounts module per auth module

NewICAModule(gov)
NewICAModule(auth 1)
NewICAModule(auth 2)

This is fine (kinda), but I think we will run into issues. Like which module binds to "interchain accounts" port and what do the others bind to?

Instead I'd like to add a router, so

router := NewRouter()
router.AddRoute("gov", gov)
router.AddRoute("auth 1", auth 1)
router.AddRoute("auth 2", auth 2)
NewICAModule(router)

The only problem is then we probably want to map, owner address -> auth module name

Thoughts @seantking ?

@seantking
Copy link
Contributor

NewICAModule(gov)
NewICAModule(auth 1)
NewICAModule(auth 2)

When you say this is the "current design", do you mean the idea you have for middleware implementation or the current ics27 spec?

Honestly, the more I think about the middleware implementation of ICA, I wonder how much of it is nice to have vs something we should do as a priority.

The primary upside of having the middleware implementation is the ability to remove the "hooks" design that currently exists, right?

I think I would be fine with leaving this middleware update of ICA to the very end and deciding then how much value the improvements will add based on us integrating into an app/testing. If only because it's going to deviate from the current design quite a bit and I'd prefer to reach parity with the current spec first (we will need to update the spec + sync with informal on the changes etc). Also, we will probably have a much clearer idea of what we expect from middleware once the 29-fee is being used.

I am open to opinions on this but I wonder if we can push the current design across the finish line and then begin working on DUX improvements as necessary?

@colin-axner
Copy link
Contributor Author

You make very good points. I do agree that the middleware is primarily allowing us to remove the hooks, but I still think that is valuable. Once the initial implementation is finalized, I think it would be disruptive to change the hooks -> middleware. It's hard to tell how much technical debt will add up from going with hooks compared to tackling middleware now

I think there's another benefit of doing the middleware for interchain accounts now though. It will inform the design of ICS30, already we have run into road bumps implementing ICS30 for relayer incentivization. I think it is very valuable to get the insight of adding this to interchain accounts which constructed in a different fashion than ics20. This might allow us to stumble upon valuable design improvements

Lastly I think the middleware design fits so naturally for the controller side that it doesn't seem out of the way

I think it'll be easier to make a decision of what to do when we can see the diffs. I'll just focus on a POC for now and flesh out the tests later once we decide what direction to take

@colin-axner
Copy link
Contributor Author

It might be worth noting, ICA will need changes to be compatible with ICS29. This seems very much worth doing

@seantking
Copy link
Contributor

seantking commented Sep 16, 2021

I think it'll be easier to make a decision of what to do when we can see the diffs. I'll just focus on a POC for now and flesh out the tests later once we decide what direction to take

Sounds good. POC first sounds like a good idea to me 👍 My only fear is blocking on this due to unforeseen complications but doing POC + review first before committing is a good trade-off.

I do like the idea of having as many IBC applications as possible being middleware where possible, it's definitely a nice design pattern generally.

@colin-axner
Copy link
Contributor Author

closed by #417

CosmosCar pushed a commit to caelus-labs/ibc-go that referenced this issue Nov 6, 2023
Instead of `time.Sleep` a channel is used for reliable synchronization
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

3 participants