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

[DRAFT][TEST-12] Add DelegateOnHost ICA call #12

Closed
wants to merge 11 commits into from
Closed

Conversation

asalzmann
Copy link
Contributor

@asalzmann asalzmann commented May 15, 2022

Summary
Add DelegateOnHost function to send an ICA MsgDelegate tx. This is an outline of how the logic could work, but isn't functional yet. There are a number of issues to resolve (tickets marked as blockers on TEST-12 on JIRA). Some high level questions

  • How will we know the amount to stake?
  • How will we handle a tx failure?
  • Does this port schema make sense?
  • What is the connectionId? Can there be multiple connections between zone pairs?

For more detail, see https://stridelabs.atlassian.net/jira/software/projects/TEST/boards/2?selectedIssue=TEST-12

Test plan
draft PR

Copy link
Contributor

@riley-stride riley-stride left a comment

Choose a reason for hiding this comment

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

Leaving a few quick/early comments on this one, seems like it's still in DRAFT

// we could make these fields repeated for more granularity, if we want
ICAAccount rewardsAccount = 5;
ICAAccount feeAccount = 6;
ICAAccount delegationAccount = 8;
Copy link
Contributor

Choose a reason for hiding this comment

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

I was holding we'd keep a list of delegatoinAccounts in case we want to move to governance pass through later. You thinking it's easier to start with a single account on each host zone for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think for the MVP it might be easier to start with one (because we won't have pass through governance at launch). Even if we do launch with multiple I think we should push that to a later ticket (just so that we can get something working). Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed!

// How can we fetch chainId here?
chainId := "hardcoded-chainId"
// Sanity check the output of account.GetTarget() (not sure if it prints an int or a stringified ICAAccountType)
owner := fmt.Sprintf("%s-%s-%s", account.GetAddress(), chainId, account.GetTarget().String())
Copy link
Contributor

Choose a reason for hiding this comment

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

Does GetTarget() need to be implemented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It gets auto-generated for us

)

// SubmitTx submits an ICA transaction
// NOTE: this is not a standard message; only the stakeibc module can call this function
Copy link
Contributor

Choose a reason for hiding this comment

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

How are we enforcing "only the stakeibc module can call this function"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not in the message server so clients can't call it (fairly sure). There are no formal checks (but it's not being called by any other modules and would require a code update for that to be the case)


// RegisterAccount registers an ICA account on behalf of the stakeibc module
// NOTE: this is not a standard message; only the stakeibc module can call this function
func (k Keeper) RegisterAccount(goCtx context.Context, connectionId string, owner string) (*types.ICAAccount, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is RegisterAccount included in this commit? Seems like it would be used in InitGenesis rathre than DelegateOnHost. Perhaps you're planning to add logic that opens an ICA account on a hostZone if one does not yet exist when DelegateOnHost is called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I was planning on, but regardless this is just a function - it's being called anywhere yet (even if it's only used in InitGenesis, it needs to be defined somewhere)

@asalzmann
Copy link
Contributor Author

asalzmann commented May 26, 2022

ICA calls are now working. Note, they still need to be invoked by a client. There were a number of bug fixes / additions

bugs fixed

  • merge in changes to fix make init
  • move app.ICAControllerKeeper above app.StakeibcKeeper in app.go (as it was defined below app.StakeibcKeeper, app.ICAControllerKeeper was uninitialized as it was passed into app.StakeibcKeeper
  • add ALLOW_MESSAGES to genesis.json for the host chain so that ICA calls are enabled
  • replace github.com/cosmos/cosmos-sdk => github.com/Stride-Labs/cosmos-sdk v0.45.4-debug-2 for easier debugging

updates include
for part 2

  • update module_ibc.go to implement the ICA interface
  • Add stride/scripts/register_ica.sh to track ICA commands (should be cleaned up and added to the Makefile as an optional test)
  • add RegisterAccount, SubmitTx to tx.proto, the message server, the CLI (transactions and queries), register in codec

for part 3

  • Add ICAAccountType enum to differentiate between ICA accounts
  • Add BeginBlocker to stakeibc module
  • Add DelegateOnHost, SubmitTx, RegisterAccount functions
  • Add a DelegateInterval

Going to close this in favor of breaking this PR up into 3 distinct pieces

  1. Fix bugs
  2. Add RegisterAccount and SubmitTx commands so that we can manually test out ICA commands
  3. Update BeginBlocker to call ICA staking on behalf of the module account

@asalzmann asalzmann closed this May 26, 2022
riley-stride pushed a commit that referenced this pull request Sep 5, 2022
Fill out emoney-3 chain information
sampocs pushed a commit that referenced this pull request Jan 5, 2023
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.

2 participants