Account migration #206

asalzmann · 2022-08-20T22:25:00Z

closes: #155

What is the purpose of the change

Context
stakeibc's module account is restricted in taking certain actions, such as IBC transferring tokens (because it is blacklisted). In upcoming versions of IBC, blacklisted accounts won't be able to transfer tokens; in the current version, tokens can be transferred, then stranded upon refund. poolparty-v3 currently has tokens stuck in this state. See #155 for more details.

Structure
This diff adds a module account for each host zone. This structure was inspired by the way Osmosis creates accounts for pools (one account per pool). We could create a single account that handles all tokens (essentially a mirror of stakeibc's module account, but there's no natural place to create the account, and account creation in RegisterHostZone is natural. This also sets us up to sweep token balances, if we decide to move away from DepositRecords.

Creating accounts
[NewAccount](https://docs.cosmos.network/main/modules/auth/04_keepers.html) creates a new account (but does not store it to state). To create new module accounts, per ADR-28, we can construct keys using the module name, the purpose of the account, and a unique key for the account. In our case, the inputs are "stakeibc", "zone" and chain id (chain ids are unique per-host zone).

Brief Changelog

Add HostZone.Address
Add NewZoneAddress function to get bech32 addresses for a host zone, based on chain id
Update expected keepers to include NewAccount and SetAccount
transfer stTokens tokens to the zone account in RedeemStake
transfer native tokens to the zone account in LiquidStake
ibc transfer stTokens from the zone account to the delegation ICA
use SendCoins, rather than SendCoinsFromAccountToModule and SendCoinsFromModuleToAccount (SendCoins is more restrictive, so should be safer than using SendCoinsFrom...`)

Testing and Verifying

Integration tests

refunding tokens from the escrow account

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? no
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? no
How is the feature or change documented? na
Does this pull request update existing proto field values (and require a backend and frontend migration)? yes
Does this pull request change existing proto field names (and require a frontend migration)? no

sampocs · 2022-08-23T20:39:28Z

@asalzmann reviewing this right now. Can you give me a one sentence explanation for the switch from txMsgData to ack in the callbacks?

asalzmann · 2022-08-24T00:20:36Z

thank you for the review @sampocs ! the one-liner is that transfer acks can't be unmarshalled into TxMsgDatas, so I pushed unmarshalling acks down the stack

sampocs

Sorry Aidan got side tracked on this. I'll submit this first batch of nit comments now so I don't keep you waiting. Tomorrow morning I'll review the core functions at a higher level and review the upgrade stuff!

proto/records/callbacks.proto

x/icacallbacks/keeper/keeper.go

sampocs · 2022-08-23T19:15:53Z

x/records/keeper/callbacks.go

@@ -0,0 +1,43 @@
+package keeper


Is this considered an ICA callback? Seems like it's just a normal IBC transfer, and if so, I think the naming of these functions are a little misleading.
If you think it makes sense to consider this an icacallback for the sake of consistency, then maybe we should rename this file to icacallbacks.go to keep it consistent with stakeibc?

agreed, the naming is confusing - everything should ideally just be called callbacks. I can rename as a final commit to this PR

What do you think about calling these ibccallbacks and then renaming stakeibc/callbacks.go to icqcallbacks?

x/icacallbacks/keeper/keeper.go

x/icacallbacks/types/errors.go

x/stakeibc/keeper/msg_server_register_host_zone.go

app/upgrades/v2/upgrades.go

x/records/keeper/keeper.go

sampocs

Good stuff! This wound up being much more complex than I would have anticipated! My main questions revolve around the ack handling between stakeibc and records (I’m most definitely missing something as I’m a little less fluent than you with the middleware).

My (likely mistaken) understanding of the flow is:

Regardless of the ack type, we first try to handle it in the Records module’s OnAckPacket:
- If the ack was from the IBC transfer, we handle it appropriately [here - CallRegisteredICACallback] but then still pass the ack down to stakeibc's handler here (maybe a bug?)
- If the ack was from the other ICAs, then we pass it through to stakeibc. However, it’s unclear to me how CallRegisteredICACallback (linked above) doesn’t fail when it’s called with any ack other than the IBC transfer
- And if it’s the case that this CallRegisteredICACallback can handle all ack types (rather than just transfer), then why would there be any need for ack handling in stakeibc?

To phrase the question a little differently, is this - CallRegisteredICACallback expected to handle all ack/callback types (transfer, delegate, undelegate, etc. )?

If yes, then why do we need stakeibc’s OnAckPacket?
If no, then wouldn’t it error on all non-transfer acks before they can propagate to stakeibc?

Additional question:

If we handle the transfer ack in record’s OnAckPacket, do we need to move the unmarshalling of TxMsgData into each individual callback function? Since stakeibc's OnAckPacket should only receive the non-transfer callbacks (which will always have txMsgData in the ack), it seems like we could just keep the unmarshalling where it was in stakeibc's OnAckPacket.
Can we handle all acks in the same OnAckPacket? Maybe in stakeibc’s since I think it has access to the records keeper.

Apologies for the confusion / long winded response - happy to hop on a call if it makes it easier!

x/records/module_ibc.go

x/icacallbacks/module_ibc.go

x/records/module_ibc.go

app/upgrades/v2/upgrades.go

asalzmann · 2022-08-25T17:04:45Z

Good stuff! This wound up being much more complex than I would have anticipated! My main questions revolve around the ack handling between stakeibc and records (I’m most definitely missing something as I’m a little less fluent than you with the middleware).

...

Great question! I think this is the key misunderstanding

Regardless of the ack type, we first try to handle it in the Records module’s OnAckPacket:

We don't pass all acks through records, rather we have two middleware _stacks_, which we route IBC packets through. The stacks are defined here

The first stack is the _records stack_, which consists of records -> transfer. The second stack is the _stakeibc_ stack, which consists of ICAController -> icacallbacks -> stakeibc. Actually, I think we should add icacallbacks between records and transfer!

The way IBC middlewares work, received packets flow from the IBC router down the stack towards the base app; sent packets flow in the opposite direction. Packets are routed to middlewares (or modules, if no middleware is defined!) through the IBC router. The router is defined here. Notably, IBC packets sent from the transfer module are routed to the records stack, and IBC packets sent from the ICA module are routed to the icamiddleware stack (which contains stakeibc).

The incoming packet flows look something like this

packet{sourcePort: transfer, ...} -> ibcRouter.Route(packet) -> records.Ack(packet) -> transfer.Ack(packet) -> baseApp.Ack(packet)

packet{sourcePort: icacontroller, ...} -> ibcRouter.Route(packet) -> icacontroller.Ack(packet) -> icacallbacks.Ack(packet) -> stakeibc.Ack(packet) -> baseApp.Ack(packet)

asalzmann · 2022-08-25T17:22:17Z

If we handle the transfer ack in record’s OnAckPacket, do we need to move the unmarshalling of TxMsgData into each individual callback function? Since stakeibc's OnAckPacket should only receive the non-transfer callbacks (which will always have txMsgData in the ack), it seems like we could just keep the unmarshalling where it was in stakeibc's OnAckPacket.

This would be true if we didn't have an interface for ICACallbackHandler. CallICACallback requires a consistent signature across modules, which is why I generalized to acks and pushed module specific ack parsing down the stack. Let me know if you can think of a cleaner solution! I think this likely works for now though, and we can always refactor this shortly after launch.

Can we handle all acks in the same OnAckPacket? Maybe in stakeibc’s since I think it has access to the records keeper.

I think this was answered in the above response, but in short: it's tricky, because the middleware stacks are different. The original reason we created separate stacks was that the Transfer module's IBCModule interface is different from ICAControllerModule's, so you can't route ICS-20 and ICS-27 packets to the same IBCModule (see the UNIMPLEMENTED methods in module_ibc.go).

Also, please don't apologize! This is a great question, and explaining it uncovered a minor bug :D

… keeper, add IBCKeeper to records module

sampocs · 2022-08-25T18:17:27Z

@asalzmann You're the man! I appreciate the detailed response! That clears it up a ton. I'm also aligned on having the callbacks use an ack instead of txMsgData

sampocs

Changes look good!

sampocs · 2022-08-25T18:21:17Z

x/stakeibc/keeper/msg_server_liquid_stake_test.go

@@ -16,6 +16,7 @@ const (
 	atom    = "uatom"
 	stAtom  = "stuatom"
 	ibcAtom = "ibc/uatom"
+	chainId = "GAIA"


ty! been meaning to do this for awhile!

asalzmann · 2022-08-25T18:47:40Z

filed two follow-ons here https://stridelabs.atlassian.net/jira/software/projects/TEST/boards/2/backlog?selectedIssue=TEST-175

* create paths json schema * create akash network paths file * add tag description * akash paths v2 * paths schema v2 * "_v2" is the decided upon schema * change "order" to "ordering"

Co-authored-by: vish-stride <vishal@stridelabs.co>

github-actions bot added C:proto C:stakeibc dependencies Pull requests that update a dependency file C:app-wiring C:records labels Aug 20, 2022

asalzmann mentioned this pull request Aug 22, 2022

upgrade v2 #212

Merged

asalzmann force-pushed the account-migration branch from abb3576 to c46170e Compare August 24, 2022 00:11

github-actions bot added the T:build label Aug 24, 2022

sampocs reviewed Aug 24, 2022

View reviewed changes

x/records/module_ibc.go Show resolved Hide resolved

x/icacallbacks/module_ibc.go Show resolved Hide resolved

x/records/module_ibc.go Show resolved Hide resolved

sampocs reviewed Aug 24, 2022

View reviewed changes

app/upgrades/v2/upgrades.go Outdated Show resolved Hide resolved

asalzmann and others added 13 commits August 25, 2022 13:31

add zone account to host zone

75f92d4

calls to liquid stake should send native tokens to zone address

aea6d2c

calls to redeem stake should send stTokens to zone address

8787457

IBC transfers should send tokens from zone address

8ae962b

move TransferKeeper to records module, add Transfer method to records…

ae192ae

… keeper, add IBCKeeper to records module

generalize callbacks to work with other IBC modules

3ea34cb

add transfer callback without forked ibc

d167aa1

transfer callbacks working

c012ac3

transfer callbacks working

56184ff

light upgrade scaffolding

593877d

upgrade draft

70e95e1

updates per sam

e9691f7

add, update liquid stake and redeem stake tests

974cb39

asalzmann force-pushed the account-migration branch from 4c9a3a1 to 974cb39 Compare August 25, 2022 17:34

asalzmann requested a review from sampocs August 25, 2022 17:34

github-actions bot added the C:submodules label Aug 25, 2022

Merge branch 'main' into account-migration

b3f626e

sampocs approved these changes Aug 25, 2022

View reviewed changes

asalzmann merged commit 52cd8de into main Aug 25, 2022

sontrinh16 pushed a commit to notional-labs/stride that referenced this pull request Mar 27, 2023

Account migration (Stride-Labs#206)

cf7eb2b

Co-authored-by: vish-stride <vishal@stridelabs.co>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account migration #206

Account migration #206

asalzmann commented Aug 20, 2022 •

edited

Loading

sampocs commented Aug 23, 2022 •

edited

Loading

asalzmann commented Aug 24, 2022 •

edited

Loading

sampocs left a comment

sampocs Aug 23, 2022 •

edited

Loading

asalzmann Aug 25, 2022

sampocs Aug 25, 2022

sampocs left a comment

asalzmann commented Aug 25, 2022 •

edited

Loading

asalzmann commented Aug 25, 2022 •

edited

Loading

sampocs commented Aug 25, 2022

sampocs left a comment

sampocs Aug 25, 2022

asalzmann commented Aug 25, 2022

Account migration #206

Account migration #206

Conversation

asalzmann commented Aug 20, 2022 • edited Loading

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

sampocs commented Aug 23, 2022 • edited Loading

asalzmann commented Aug 24, 2022 • edited Loading

sampocs left a comment

Choose a reason for hiding this comment

sampocs Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

asalzmann Aug 25, 2022

Choose a reason for hiding this comment

sampocs Aug 25, 2022

Choose a reason for hiding this comment

sampocs left a comment

Choose a reason for hiding this comment

asalzmann commented Aug 25, 2022 • edited Loading

asalzmann commented Aug 25, 2022 • edited Loading

sampocs commented Aug 25, 2022

sampocs left a comment

Choose a reason for hiding this comment

sampocs Aug 25, 2022

Choose a reason for hiding this comment

asalzmann commented Aug 25, 2022

asalzmann commented Aug 20, 2022 •

edited

Loading

sampocs commented Aug 23, 2022 •

edited

Loading

asalzmann commented Aug 24, 2022 •

edited

Loading

sampocs Aug 23, 2022 •

edited

Loading

asalzmann commented Aug 25, 2022 •

edited

Loading

asalzmann commented Aug 25, 2022 •

edited

Loading