TEST-7: add exchange rate #17

riley-stride · 2022-05-17T21:31:21Z

DO NOT MERGE, LOGIC NOT FINISHED YET

Summary:

Implemented logic to calculate the exchange rate between stToken/Token at deposit and unbonding. The main complication along the way is not being able to figure out how to get return values out of asynchronous ICQ calls, so having to write the result to state rather than handle it in-memory. This required the creation of interchainstaking store variables.

We need rigorous testing in this code (it’s exploitable); this is just a v0 of the logic.

Pointers for review:

Most logic lives in stride/x/stakeibc/keeper/exchange_rate.go, where two functions live:
- getStTokenExchRate calcalates the exchange rate between stAsset/Asset using as a helper the ICQQueryHostBalance function. We use BankKeeper's GetSupply function to calculate supply of stAsset (same approach as QS to calculating stAsset supply). For deposits, inclOutstandingRewards param should be true, for withdrawals, false.
- ICQQueryHostBalance submits an ICQ call to measure the amount of tokens on host chain either across all delegation accounts, delegated, or in outstanding accrued rewards. Results are written to the host_zone state rather than returned (due to technical limitations described in summary).
In proto/stakeibc/host_zone.proto, added connectionID, denominations and state variables used to calc exchange rate (they're updated to measure host zone delegations and balances). Removed rewardsAccount.
In stride/proto/stakeibc/params.proto, remove unused ica_account.proto import
In stride/x/stakeibc/keeper/exchange_rate.go, replace BinaryCodec with Codec (the former does not allow for cdc.UnmarshalJSON calls required in exchange_rate calc, the latter does. Also, add bankKeeper for exchange_rate stAsset supply calcs.
Create stride/x/stakeibc/keeper/zones.go, containing helpers for zones. We could move these functions to zones.go to simplify. <==**TODO move to zones.go **
In stride/x/stakeibc/types/keys.go, added prefixes. We may not need these. <==**TODO remove these **

For more details see Jira TEST-7

Note I had to add third_party folder to get this working

asalzmann · 2022-05-17T21:47:25Z

Can you add more detail in the summary on the specific files / functions to pay attention to in this PR? It would make it easier for me to give a good review, it's hard to do so when so many LOC have changed (especially when the logic is sensitive).

Thought Vishal did a good job of this here as an example! #14

riley-stride · 2022-05-17T22:02:42Z

Is the content of TEST-7 in Jira what you're looking for? https://stridelabs.atlassian.net/jira/software/projects/TEST/boards/2?selectedIssue=TEST-7. Generally been putting much more detail there than the PRs but happy to copy over or format like Vishal's #14 if that makes things easier

riley-stride · 2022-05-17T22:11:30Z

This should be a cleaner PR now that I moved third_party to its own branch

asalzmann · 2022-05-18T10:24:16Z

Ticket has the high level context yeah - a couple of bullet point style pointers from the ticket tasks to files / places in code that implement the relevant logic would still be helpful so that I know which parts of the code require the most attention as a reviewer (especially on large PRs that have >1k LOC)

riley-stride · 2022-05-19T15:16:50Z

Ticket has the high level context yeah - a couple of bullet point style pointers from the ticket tasks to files / places in code that implement the relevant logic would still be helpful so that I know which parts of the code require the most attention as a reviewer (especially on large PRs that have >1k LOC)

Thanks this makes a lot of sense. Updated the comments.

asalzmann

Cool to see this coming together! Realize this is a draft but left a few comments

asalzmann · 2022-05-30T22:39:15Z

stride/x/stakeibc/types/keys.go

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


I think iota just resolves to 0, are all of these keys 1? Should they be strings?

asalzmann · 2022-05-30T22:40:23Z

stride/proto/stakeibc/host_zone.proto

-  repeated Validator blacklistedValidators = 4;
-  repeated ICAAccount rewardsAccount = 5;
-  repeated ICAAccount feeAccount = 6;  
+  uint64 id = 1;


Changing proto field ids around like this is risky in production (had a similar comment on another PR that went into depth here) but probably ok given we haven't launched any testnet / mainnet yet

asalzmann · 2022-05-30T22:41:31Z

stride/proto/stakeibc/host_zone.proto

+  string channelId = 4; 
+  string connectionID = 5;
+  repeated Validator validators = 6;
+  repeated ICAAccount delegationAccounts = 7;


Do we need a rewardsAccount and a feeAccount here? Also, commented on another PR that all of these ICAAccounts should be singular fields (I think)

asalzmann · 2022-05-30T22:42:14Z

stride/proto/stakeibc/host_zone.proto

+    (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Dec",
+    (gogoproto.nullable) = false
+  ];
+  string totalAllBalances = 12 [


Suggested change

string totalAllBalances = 12 [

string totalBalances = 12 [

asalzmann · 2022-05-30T22:44:28Z

stride/x/stakeibc/keeper/exchange_rate.go

+	}
+
+	switch query_type {
+	case "totalBalance":


Where does the syntax for these queries come from? Looks like we're using camelcase?

asalzmann · 2022-05-30T22:45:41Z

stride/x/stakeibc/keeper/exchange_rate.go

+)
+
+// is this the right keeper?
+func (k Keeper) getStTokenExchRate(ctx sdk.Context, hostZone types.HostZone, inclOutstandingRewards bool) (sdk.Dec, error) {


Suggested change

func (k Keeper) getStTokenExchRate(ctx sdk.Context, hostZone types.HostZone, inclOutstandingRewards bool) (sdk.Dec, error) {

func (k Keeper) updateStTokenExchRate(ctx sdk.Context, hostZone types.HostZone, inclOutstandingRewards bool) (sdk.Dec, error) {

suggest this name change so that it's clear this updates the store, and the exchange rate can then be fetched from the store

asalzmann · 2022-05-30T22:47:26Z

stride/x/stakeibc/keeper/exchange_rate.go

+	supplyOfStTokens := k.bankKeeper.GetSupply(ctx, hostZone.StDenom)
+	k.Logger(ctx).Info("stAsset outstanding supply:", supplyOfStTokens)
+
+	exchRate := balOfVirtualPoolOnHost.Quo(supplyOfStTokens.Amount.ToDec())


Rather than returning an exchange rate, should this just update a variable in the store? That way, we could put this call in the BeginBlocker or EndBlocker (or even in an epoch) and just update the exchange rate on some cadence

asalzmann · 2022-05-30T22:48:35Z

stride/x/stakeibc/keeper/exchange_rate.go

+			k.Logger(ctx).Error("Unable to unmarshal validators info for zone", "zone", zone.ChainId, "err", err)
+			return err
+		}
+		hostZone.TotalAllBalances = sdk.Dec(queryRes.Balances.AmountOf(hostZone.BaseDenom))


I'm not sure this update will have any effect on the store (if that's the intention), I think you need to update the variable then set it in the store.

Also not totally sure whether this variable is scoped to the cbTotalBalance function or if updates persist outside that scope

riley-stride · 2022-06-10T06:31:00Z

Closing this as it's subsumed by TEST-50

* Fix CI * Remove on push due to double run * Update naming * Try adding an error * For some reason commenting seems broken * CI errors worked!

Unbonding Record Keepers

riley-stride added 4 commits May 16, 2022 12:30

TEST-7 exchange rate calc pseudocode

268fe6a

TEST-7 add first try calc'ing amt delegated using ICQ

9f470b4

TEST-7 added stToken supply logic and exch rate logic, untested

7648546

TEST-7 merge with master

b8f3b3c

riley-stride requested review from shellvish and asalzmann and removed request for shellvish May 17, 2022 21:31

TEST-7 rm third_party from this PR for reviewer clarity

c6e7c01

Merge branch 'main' into TEST-7

50fee5b

riley-stride marked this pull request as draft May 27, 2022 09:55

asalzmann reviewed May 30, 2022

View reviewed changes

riley-stride closed this Jun 10, 2022

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

Fix CI (#17)

a87dd0c

* Fix CI * Remove on push due to double run * Update naming * Try adding an error * For some reason commenting seems broken * CI errors worked!

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

Merge pull request #17 from Stride-Labs/sam/sttia-unbonding-records

fbab2ba

Unbonding Record Keepers

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TEST-7: add exchange rate #17

TEST-7: add exchange rate #17

riley-stride commented May 17, 2022 •

edited

Loading

asalzmann commented May 17, 2022

riley-stride commented May 17, 2022

riley-stride commented May 17, 2022

asalzmann commented May 18, 2022

riley-stride commented May 19, 2022

asalzmann left a comment

asalzmann May 30, 2022

asalzmann May 30, 2022

asalzmann May 30, 2022

asalzmann May 30, 2022

asalzmann May 30, 2022

asalzmann May 30, 2022

asalzmann May 30, 2022

asalzmann May 30, 2022

riley-stride commented Jun 10, 2022

	func (k Keeper) getStTokenExchRate(ctx sdk.Context, hostZone types.HostZone, inclOutstandingRewards bool) (sdk.Dec, error) {
	func (k Keeper) updateStTokenExchRate(ctx sdk.Context, hostZone types.HostZone, inclOutstandingRewards bool) (sdk.Dec, error) {

TEST-7: add exchange rate #17

TEST-7: add exchange rate #17

Conversation

riley-stride commented May 17, 2022 • edited Loading

asalzmann commented May 17, 2022

riley-stride commented May 17, 2022

riley-stride commented May 17, 2022

asalzmann commented May 18, 2022

riley-stride commented May 19, 2022

asalzmann left a 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 10, 2022

riley-stride commented May 17, 2022 •

edited

Loading