TEST-9: added OnChanOpenAck account host chain setup (some sub-parts blocked by new tickets) #13

riley-stride · 2022-05-16T15:41:23Z

High level summary: the first time a channel is opened to any hostZone, OnChanOpenAck will run. We add logic here to populate the HostZone store with host chain accounts. There’s one channel per account on the HostZone, so switch on portID to determine which is being opened and store the appropriate account.

See TEST-9 in Jira

Note: this PR included the addition of the third_party folder which includes several imports. We can remove it from here and add it to a separate PR if that's cleaner.

… accounts

…o to typecheck address string types ; add feeAccount back to host_zone

…blocked by new tickets)

asalzmann · 2022-05-16T15:47:48Z

Can you add a high level summary here?

riley-stride · 2022-05-16T16:41:09Z

Can you add a high level summary here?

@asalzmann added

riley-stride · 2022-05-17T22:12:37Z

This should be a cleaner commit now that third_party was moved to TEST-47 👍

asalzmann

Exciting to see this coming together! I left some comments throughout, overall the approach makes sense to me. The only major comments are

We'll need to align on a portId schema (as well as how we map between portIds and associated fields like ICAAccount type)
Whether the funcionality that's in OnChanOpenAck here should go in OnChanOpenConfirm

My understanding of the stages of opening channels is

OnChanOpenInit - A lets B know that it wants to open a channel
OnChanOpenTry - B tries to open the channel
OnChanOpenAck - A lets B know if the channel is open
OnChanOpenConfirm - if it's open, B lets A know that it also has the channel open

Not clear to me whether B could fail in step 4 and whether we should do cleanup logic if that's the case.

Before merging this, can we test this by creating a connection between two chains and checking the HostZone store?

asalzmann · 2022-05-18T16:35:00Z

stride/proto/stakeibc/genesis.proto

@@ -19,3 +19,8 @@ message GenesisState {
  uint64 hostZoneCount = 6;
  // this line is used by starport scaffolding # genesis/proto/state
 }
+
+message PortConnectionTuple {


Do we need this tuple?

asalzmann · 2022-05-18T16:35:29Z

stride/proto/stakeibc/host_zone.proto

-  repeated Validator blacklistedValidators = 4;
-  repeated ICAAccount rewardsAccount = 5;
-  repeated ICAAccount feeAccount = 6;  
+  repeated ICAAccount delegationAccounts = 4;


Reusing proto fields (the numbers next to each field) is bad practice and can break databases, because of the way protos are serialized / deserialized. The proto field number indicates where in the serialized byte-string the data is stored, so if you change the field number database may no longer work. For example, if a proto message has a field string animal = 2 and you store a string "dog" at a specific position in the serialized byte-string (via a db write), then change the proto field to int animal = 2, the next time you read that field the proto will deserialize the byte-string and try to interpret the bytes representing "dog" as an int.

Instead, use the "reserved" syntax to deprecate fields like so

reserved 4, 5, 6;

and set the new field to the next id (7 in this case) like so

repeated ICAAccount delegationAccounts = 7;

docs: https://developers.google.com/protocol-buffers/docs/proto3#assigning_field_numbers

How are you thinking about removing the fee and rewards accounts? I think we still need these, if we remove them I'm not sure where we'll send fees + rewards.

In #12 I made all of the ICAAccount fields not repeated (with the exception of delegationAccounts). I think it will simplify the code somewhat to have a single account handling fees, rewards, etc. on each HostChain, but we do lose a little bit of flexibility. How do you think we should handle these?

Fee account: I believe we need the fee account (just an addr on host chain that collects revenue from protocol, NOT an ICA acct). I added that back in on another PR. I

Rewards account: I don't think we need this because we can just sweep rewards to the deposit or delegation account and the regular epoch logic will delegate these claimed rewards from either of those accounts.

Thanks for pointing out the proto field numbring issue. Let me make sure I understand: you're suggesting that there are some web2 cases (e.g. some DBs) that break if you change the type of a particular field. It's less clear to me that we need to deprecate proto fields here given we re-compile every time. Maybe we will need to once the chain is running, so we should just follow that convention now?

It's not specific to DBs, it's specific to protos - whenever you read a proto byte-string into memory (doesn't matter where the byte-string comes from, it could be a DB, a blockchain, a csv, etc.) the only way the code can interpret that byte-string is through the current message definition, so if it was defined using an old message schema it could throw an error. Deprecating fields is fine (e.g. removing a field from the proto entirely), the byte-string will still be interpreted correctly and when it's read into memory the deprecated data in the byte-string will be ignored, but replacing fields and using the same id is error-prone (case outlined above).

It's true that technically this doesn't break anything while we're in the development pattern of restarting chain state (we clear out the "DB" every time we re-initialize the chain), but defining new field ids is cheap and forgetting this later / having to think about when we need to define new fields is expensive and error-prone, so would bias towards following the convention now regardless.

Re: the rewards account, sweeping rewards into the deposit account could be a bit complex - I think we'd need an IBC transaction (deposit account is on stride, rewards accrue on host), although we could sweep them directly into the delegation account. As long as we only have a single delegation account, I think this works fine, if we have multiple delegation accounts it could be more complex (we'd have to withdraw rewards in chunks to different delegation accounts)

asalzmann · 2022-05-18T16:37:25Z

stride/x/stakeibc/keeper/keeper.go

 )

 type (
 	Keeper struct {
 		// *cosmosibckeeper.Keeper
-		cdc        codec.BinaryCodec


I think we have different linters, we should all get in sync on using one (not important for this PR, just making a note)

Agreed, I'm using this one https://github.com/golang/vscode-go/blob/master/docs/features.md#lint-errors. Lmk if you have one you prefer

asalzmann · 2022-05-18T16:37:40Z

stride/x/stakeibc/keeper/keeper.go

@@ -78,3 +80,40 @@ func (k Keeper) Logger(ctx sdk.Context) log.Logger {
 func (k *Keeper) ClaimCapability(ctx sdk.Context, cap *capabilitytypes.Capability, name string) error {
 	return k.scopedKeeper.ClaimCapability(ctx, cap, name)
 }
+
+func (k *Keeper) SetConnectionForPort(ctx sdk.Context, connectionId string, port string) error {


This code is a bit confusing to me, should there be a single connection between any two HostZones? Rather than storing a connection for each port, could we derive the HostZone some other way? I think this works for ICAAccounts (which have a unique port per ICA), but I would think other types of ports could support multiple connections? E.g. I would guess that the transfer port on Osmosis is connected to every zone that Osmosis is connected to

asalzmann · 2022-05-18T16:37:52Z

stride/x/stakeibc/keeper/zones.go

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


Do we need a new keeper file here? Wondering if these functions go in stride/x/stakeibc/keeper/host_zone.go

asalzmann · 2022-05-18T16:39:22Z

stride/x/stakeibc/module_ibc.go

+		zoneInfo.DelegationAccounts = append(zoneInfo.DelegationAccounts, delegationAccount)
+	case portParts[1] == "feeAccount":
+		// set HostZone feeAccount
+		// TODO (TEST-) add Fee account safety checks (we can't have this changing without rigorous validation)


Missing ticket number?

Do we need an ICAAccount here? Something like feeAccount = &types.ICAAccount{Address: address, Balance: 0, DelegatedBalance: 0, Delegations: []*types.Delegation{}}

asalzmann · 2022-05-18T16:39:57Z

stride/x/stakeibc/types/keys.go

+const (
+	HostZoneKey            = "HostZone-value-"
+	HostZoneCountKey       = "HostZone-count-"
+	prefixZone             = iota + 1


@vish-stride brought up that this syntax looks off, I think this will just set all of the fields to 1? (iota is another way to represent 0 iirc)

asalzmann · 2022-05-18T16:40:12Z

stride/x/stakeibc/types/keys.go

 var (
 	// PortKey defines the key to store the port ID in store
-	PortKey = KeyPrefix("stakeibc-port-")
+	PortKey                   = KeyPrefix("stakeibc-port-")


What are these prefixes being used for?

asalzmann · 2022-05-18T16:40:20Z

stride/x/stakeibc/types/keys.go

-	PortKey = KeyPrefix("stakeibc-port-")
+	PortKey                   = KeyPrefix("stakeibc-port-")
+	KeyPrefixZone             = []byte{prefixZone}
+	KeyPrefixIntent           = []byte{prefixIntent}


I don't think we need this (don't have intents)

asalzmann · 2022-05-18T16:40:28Z

stride/x/stakeibc/types/keys.go

+	KeyPrefixIntent           = []byte{prefixIntent}
+	KeyPrefixPortMapping      = []byte{prefixPortMapping}
+	KeyPrefixReceipt          = []byte{prefixReceipt}
+	KeyPrefixWithdrawalRecord = []byte{prefixWithdrawalRecord}


Are we tracking withdrawal records?

riley-stride · 2022-06-03T13:39:17Z

This PR was rolled into #28

Update osmosis seeds to include whats in the binary

nit cleanup changes after scaffolding

riley-stride added 5 commits May 14, 2022 12:33

give host zones a delegation account and remove their fee and rewards…

5128fd1

… accounts

TEST-9: copy in third_party dependencies from other ; use cosmos_prot…

1ecb6ee

…o to typecheck address string types ; add feeAccount back to host_zone

added text outline of other behavior in OnChanOpenAck

8971b21

Merge branch 'main' of github.com:Stride-Labs/stride into TEST-9

f4c8bd7

TEST-9: added OnChanOpenAck account host chain setup (some sub-parts …

920d88a

…blocked by new tickets)

riley-stride requested review from shellvish and asalzmann May 16, 2022 15:41

TEST-9 rm third_party from this PR for reviewer clarity

f3b192b

asalzmann requested changes May 18, 2022

View reviewed changes

riley-stride closed this Jun 3, 2022

riley-stride pushed a commit that referenced this pull request Sep 5, 2022

Merge pull request #13 from osmosis-labs/dev/update_osmosis_seeds

71510c9

Update osmosis seeds to include whats in the binary

asalzmann pushed a commit that referenced this pull request Jan 25, 2024

Merge pull request #13 from Stride-Labs/scaffolding-cleanup

b03544f

nit cleanup changes after scaffolding

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TEST-9: added OnChanOpenAck account host chain setup (some sub-parts blocked by new tickets) #13

TEST-9: added OnChanOpenAck account host chain setup (some sub-parts blocked by new tickets) #13

riley-stride commented May 16, 2022 •

edited

Loading

asalzmann commented May 16, 2022

riley-stride commented May 16, 2022

riley-stride commented May 17, 2022

asalzmann left a comment

asalzmann May 18, 2022

asalzmann May 18, 2022 •

edited

Loading

asalzmann May 18, 2022

asalzmann May 18, 2022

riley-stride May 19, 2022

riley-stride May 19, 2022

asalzmann May 19, 2022 •

edited

Loading

asalzmann May 19, 2022

asalzmann May 18, 2022

riley-stride May 19, 2022

asalzmann May 18, 2022

asalzmann May 18, 2022

asalzmann May 18, 2022

asalzmann May 18, 2022

asalzmann May 18, 2022

asalzmann May 18, 2022

asalzmann May 18, 2022

asalzmann May 18, 2022

riley-stride commented Jun 3, 2022

TEST-9: added OnChanOpenAck account host chain setup (some sub-parts blocked by new tickets) #13

TEST-9: added OnChanOpenAck account host chain setup (some sub-parts blocked by new tickets) #13

Conversation

riley-stride commented May 16, 2022 • edited Loading

asalzmann commented May 16, 2022

riley-stride commented May 16, 2022

riley-stride commented May 17, 2022

asalzmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asalzmann May 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asalzmann May 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riley-stride commented Jun 3, 2022

riley-stride commented May 16, 2022 •

edited

Loading

asalzmann May 18, 2022 •

edited

Loading

asalzmann May 19, 2022 •

edited

Loading