feat(app): add upgrade handlers #1121

aleem1314 · 2022-05-23T13:50:16Z

Description

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

codecov · 2022-05-23T14:38:53Z

Codecov Report

Merging #1121 (9982038) into master (abd6de2) will decrease coverage by 0.15%.
The diff coverage is 60.60%.

❗ Current head 9982038 differs from pull request most recent head fe9dfaf. Consider uploading reports for the commit fe9dfaf to get more accurate results

@@            Coverage Diff             @@
##           master    #1121      +/-   ##
==========================================
- Coverage   69.52%   69.36%   -0.16%     
==========================================
  Files         218      219       +1     
  Lines       22578    22735     +157     
==========================================
+ Hits        15697    15771      +74     
- Misses       5527     5597      +70     
- Partials     1354     1367      +13

Flag	Coverage Δ
experimental-codecov	`69.39% <67.77%> (+0.01%)`	⬆️
stable-codecov	`63.68% <60.60%> (-0.21%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

app/stable_appconfig.go

x/ecocredit/migrations/v3/core.go

x/ecocredit/migrations/v3/patch.go

ryanchristo

This is looking good. A few requested changes. I think separating the tests for manual operations would be nice so that we are not only testing mainnet and the tests in core_test.go would test a scenario with no matching chain id. Would be easier to add cases for manual operations that are not being tested at the moment and to do so specific to each chain.

app/recover_funds_test.go

app/stable_appconfig.go

x/ecocredit/migrations/v3/core_test.go

x/ecocredit/migrations/v3/patch.go

anilcse

why don't we take a complete new account for recovering funds? In that way, we can just replace old account address, pubkey with new one in the account state and bank state

…-network/regen-ledger into aleem/980-upgrade-handlers

Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>

…-network/regen-ledger into aleem/980-upgrade-handlers

ryanchristo

This looks great. I was think that the tests in patch_test.go wouldn't need to set up and run the state migrations (not including the manual operations) and then they would only include testing the manual operations but how you set them up works too and is more thorough so I think we should keep it as is.

Items remaining in this pull request:

consolidate into one upgrade handler

Items remaining that we can address in a followup pull request:

update community member address (we need an address that does not yet exist on chain)
add reference ids and update reference ids and issuance dates for any new batches created

app/stable_appconfig.go

ryanchristo · 2022-05-27T17:18:58Z

app/stable_appconfig.go

+	// address that the community member has access to
+	const newAddr = "regen14tpuqrwf95evu3ejm9z7dn20ttcyzqy3jjpfv4"


As discussed on our call, this account has an existing balance and we need an address that does not have a balance and does not exist yet on chain so that we can create a vesting account for the address.

balances: - amount: "1504879" denom: uregen pagination: next_key: null total: "0"

cc @clevinson @dpdanpittman

Following up on this... @ryanchristo and I just spent some time reviewing this PR in more detail (in particular the recoverFunds() implementation), and I think this may work as desired even if the user has a pre-existing balance.

The error when setting a periodic vesting account on a pre-existing account only happens in the message server for CreateVestingAccount().

So I think the way we are doing it (by ensuring that the vesting acc balance is added to the pre-existing new account balance), and then calling accountKeeper.SetAccount() should avoid an issues.

See #1121 (comment) and reply.

x/ecocredit/migrations/v3/patch.go

ryanchristo · 2022-05-27T17:25:38Z

x/ecocredit/migrations/v3/patch.go

+	// project location -> reference-id
+	// FR              -> "" // TODO: add reference-id
+	// US              -> ""
+	// AU-NSW 2453     -> ""
+	// K               -> ""
+	// US-FL 9876      -> ""


Same as above. Will check with @S4mmyb on reference id for projects created on redwood.

My guess is that we can leave reference ID blank on redwood.

Sam is checking with the interface team. They may want some of these projects to have a reference id but we can handle in a followup.

Added this as a followup to the issue so that we are not beta blocking: #980

We can move forward without including any sort of reference id for projects on redwood @ryanchristo

Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>

app/stable_appconfig.go

ryanchristo

Successfully tested software upgrade without chain id regen-1 or regen-redwood-1. Will need to do more thorough testing using existing mainnet state and redwood state but this looks good to me and the remaining items can be addressed in a followup.

Also, one issue came up but unrelated to the changes here, which is issuer was not populated in the new credit batch following migrations (opened #1138).

app/recover_funds_test.go

clevinson · 2022-06-02T21:02:11Z

x/ecocredit/migrations/v3/patch.go

+	locationToReferenceIdMap["CD-MN"] = ""
+	locationToReferenceIdMap["KE"] = ""


Suggested change

locationToReferenceIdMap["CD-MN"] = ""

locationToReferenceIdMap["KE"] = ""

locationToReferenceIdMap["CD-MN"] = "VCS-934"

locationToReferenceIdMap["KE"] = "VCS-612"

@S4mmyb what do you think of this for the mai ndombw and kasigao reference ID's?

Mai Ndombe: VCS-934 (https://registry.regen.network/projects/mai-ndombe)

Kasigao: VCS-612 (https://registry.regen.network/projects/kasigau)

Committing this change and merging this pull request so that we can tag a beta and test. If we need to make any revisions, we can do so at a later point in time. Updated the issue and added followups: #980

x/ecocredit/migrations/v3/patch_test.go

Co-authored-by: Cory <cjlevinson@gmail.com>

anilcse

lgtm, couple of notes for your consideration.

app/stable_appconfig.go

anilcse · 2022-06-03T09:34:14Z

app/stable_appconfig.go

+	va, ok := lostAccount.(*vestingtypes.PeriodicVestingAccount)
+	if !ok {
+		return fmt.Errorf("%s is not a vesting account", lostAddr)
+	}
+
+	vestingPeriods := va.VestingPeriods
+	// unlock vesting tokens
+	newVestingPeriods := make([]vestingtypes.Period, len(va.VestingPeriods))
+	for i, vp := range va.VestingPeriods {
+		vp.Length = 0
+		newVestingPeriods[i] = vp
+	}
+	va.VestingPeriods = newVestingPeriods
+	ak.SetAccount(ctx, va)
+
+	// send spendable balance from lost account to new account
+	spendable := bk.SpendableCoins(ctx, lostAccount.GetAddress())
+	if err := bk.SendCoins(ctx, lostAccount.GetAddress(), newAccount.GetAddress(), spendable); err != nil {
+		return err
+	}
+
+	newPVA := vestingtypes.NewPeriodicVestingAccount(
+		authtypes.NewBaseAccount(newAccount.GetAddress(), newAccount.GetPubKey(), newAccount.GetAccountNumber(), newAccount.GetSequence()),
+		va.OriginalVesting, va.StartTime, vestingPeriods,
+	)
+	ak.SetAccount(ctx, newPVA)


This works all fine but looks complex for me. I was advising to consider a brand new account from user for simplicity. The solution can simply be

// read lost-account from auth store acc := regenApp.AccountKeeper.GetAccount(ctx, lostAddr) newAcc := acc; newAcc.Address = newAddr newAcc.Pubkey = nil // delete old account err := regenApp.AccountKeeper. RemoveAccount(ctx, lostAcc) // set new account err := regenApp.AccountKeeper.SetAccount(ctx, newAcc)

similarly for bank balances too. Not sure how this sounds for others but I am comfortable with this approach.

Yea, this would be easier.

Given the community member is not necessarily an experience blockchain user and knowing the address provided is something they have access to, I think the extra work on our end is ok. I'm going to merge as is so that we can cut a beta and start more thorough testing but I added this as a potential followup to the issue: #980

…-network/regen-ledger into aleem/980-upgrade-handlers

Co-authored-by: Cory <cjlevinson@gmail.com>

ryanchristo · 2022-06-03T17:01:46Z

Very nice work @aleem1314 🚀

feat: wip mainnet upgrade handler

adccebe

aleem1314 added 5 commits May 24, 2022 15:59

wip: add ecocredit state migrations

cd62000

implement recover funds from lost account

5186804

chore: fix tests

fd24f1b

chore: add test

94f830d

chore: fix test

82bc8cc

aleem1314 commented May 25, 2022

View reviewed changes

app/stable_appconfig.go Outdated Show resolved Hide resolved

Update app/stable_appconfig.go

0e33ef0

aleem1314 marked this pull request as ready for review May 25, 2022 14:54

aleem1314 requested review from ryanchristo, technicallyty and anilcse May 25, 2022 14:55

aleem1314 added 2 commits May 25, 2022 20:26

chore: cleanup

0dc37be

chore: cleanup

4eaff10

aleem1314 commented May 25, 2022

View reviewed changes

x/ecocredit/migrations/v3/core.go Outdated Show resolved Hide resolved

feat: add redwood migrations

02fedd8

aleem1314 commented May 26, 2022

View reviewed changes

x/ecocredit/migrations/v3/patch.go Outdated Show resolved Hide resolved

Update x/ecocredit/migrations/v3/patch.go

ccfa0f8

ryanchristo reviewed May 26, 2022

View reviewed changes

x/ecocredit/migrations/v3/patch.go Outdated Show resolved Hide resolved

anilcse reviewed May 27, 2022

View reviewed changes

aleem1314 and others added 7 commits May 27, 2022 18:01

feat: add redwood migration tests

70b58e8

Merge branch 'aleem/980-upgrade-handlers' of https://github.com/regen…

8efe829

…-network/regen-ledger into aleem/980-upgrade-handlers

chore: revert core_test.go

6747aa6

Update app/stable_appconfig.go

11dfd85

Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>

Merge branch 'aleem/980-upgrade-handlers' of https://github.com/regen…

0004544

…-network/regen-ledger into aleem/980-upgrade-handlers

chore: review changes

9ed5b5b

chore: add redwood upgrade handler

765fb4f

ryanchristo reviewed May 27, 2022

View reviewed changes

Update app/stable_appconfig.go

61aceda

Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>

aleem1314 commented May 28, 2022

View reviewed changes

app/stable_appconfig.go Outdated Show resolved Hide resolved

Update app/stable_appconfig.go

571c3b8

anilcse reviewed May 31, 2022

View reviewed changes

app/stable_appconfig.go Show resolved Hide resolved

ryanchristo approved these changes May 31, 2022

View reviewed changes

anilcse and others added 2 commits June 2, 2022 20:57

Merge branch 'master' into aleem/980-upgrade-handlers

44ef5b7

Merge branch 'master' into aleem/980-upgrade-handlers

9dd7f37

ryanchristo requested a review from anilcse June 2, 2022 17:00

clevinson approved these changes Jun 2, 2022

View reviewed changes

aleem1314 and others added 4 commits June 3, 2022 11:46

Update x/ecocredit/migrations/v3/patch_test.go

02af4e4

Co-authored-by: Cory <cjlevinson@gmail.com>

Update x/ecocredit/migrations/v3/patch_test.go

0f4aaa5

Co-authored-by: Cory <cjlevinson@gmail.com>

Update x/ecocredit/migrations/v3/patch_test.go

343af4a

Co-authored-by: Cory <cjlevinson@gmail.com>

Update x/ecocredit/migrations/v3/patch_test.go

50d012b

Co-authored-by: Cory <cjlevinson@gmail.com>

anilcse approved these changes Jun 3, 2022

View reviewed changes

aleem1314 added 4 commits June 3, 2022 15:19

chore: review changes

7ecc794

Merge branch 'aleem/980-upgrade-handlers' of https://github.com/regen…

168face

…-network/regen-ledger into aleem/980-upgrade-handlers

Merge branch 'master' into aleem/980-upgrade-handlers

372f378

chore: fix errors

d6bc0f2

ryanchristo mentioned this pull request Jun 3, 2022

Upgrade handlers and manual state operations #980

Closed

11 tasks

ryanchristo and others added 3 commits June 3, 2022 08:38

Update x/ecocredit/migrations/v3/patch.go

2e0def5

Co-authored-by: Cory <cjlevinson@gmail.com>

update reference id comments and checks

9982038

Merge branch 'master' into aleem/980-upgrade-handlers

fe9dfaf

ryanchristo merged commit 1abcee8 into master Jun 3, 2022

ryanchristo deleted the aleem/980-upgrade-handlers branch June 3, 2022 17:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(app): add upgrade handlers #1121

feat(app): add upgrade handlers #1121

aleem1314 commented May 23, 2022 •

edited by aaronc

Loading

codecov bot commented May 23, 2022 •

edited

Loading

ryanchristo left a comment

anilcse left a comment

ryanchristo left a comment •

edited

Loading

ryanchristo May 27, 2022

ryanchristo May 27, 2022

clevinson Jun 2, 2022

ryanchristo Jun 3, 2022 •

edited

Loading

ryanchristo May 27, 2022

clevinson Jun 2, 2022

ryanchristo Jun 2, 2022

ryanchristo Jun 3, 2022

S4mmyb Jun 7, 2022

ryanchristo left a comment

clevinson Jun 2, 2022

ryanchristo Jun 3, 2022

anilcse left a comment

anilcse Jun 3, 2022

ryanchristo Jun 3, 2022 •

edited

Loading

ryanchristo commented Jun 3, 2022

		// address that the community member has access to
		const newAddr = "regen14tpuqrwf95evu3ejm9z7dn20ttcyzqy3jjpfv4"

		locationToReferenceIdMap["CD-MN"] = ""
		locationToReferenceIdMap["KE"] = ""

feat(app): add upgrade handlers #1121

feat(app): add upgrade handlers #1121

Conversation

aleem1314 commented May 23, 2022 • edited by aaronc Loading

Description

Author Checklist

Reviewers Checklist

codecov bot commented May 23, 2022 • edited Loading

Codecov Report

ryanchristo left a comment

Choose a reason for hiding this comment

anilcse left a comment

Choose a reason for hiding this comment

ryanchristo left a comment • 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

ryanchristo Jun 3, 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

ryanchristo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anilcse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanchristo Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

ryanchristo commented Jun 3, 2022

aleem1314 commented May 23, 2022 •

edited by aaronc

Loading

codecov bot commented May 23, 2022 •

edited

Loading

ryanchristo left a comment •

edited

Loading

ryanchristo Jun 3, 2022 •

edited

Loading

ryanchristo Jun 3, 2022 •

edited

Loading