[Application] chore: implement app transfer period #789

bryanchriswhite · 2024-09-04T09:12:04Z

Summary

Reconcile the need for a transfer period with [Application] feat: app stake transfer #743.
Ensure the app transfer message handler returns grpc status errors consistently.
Improve some shared E2E tests.
Implement ApplicationIntegrationSuite test suite.
Port app transfer E2E tests to integration tests.

Depends on

Issue

[Utility][Morse Parity] Staked App Transfer #657

Type of change

Select one or more:

Testing

Documentation: make docusaurus_start; only needed if you make doc changes
Unit Tests: make go_develop_and_test
LocalNet E2E Tests: make test_e2e
DevNet E2E Tests: Add the devnet-test-e2e label to the PR.

Sanity Checklist

I have tested my changes using the available tooling
I have commented my code
I have performed a self-review of my own code; both comments & source code
I create and reference any new tickets, if applicable
I have left TODOs throughout the codebase, if applicable

…eficiary:string --module application

Co-authored-by: Daniel Olshansky <[email protected]>

…ke-transfer * pokt/main: [Application] Implement unbonding period (#735) [Docs] Move over docs from poktroll-docker-compose-example (#757) [Performance] Reduce RelayMiner memory consumption under load (#739)

…ssions

tests/integration/application/application_transfer_test.go

x/application/keeper/transfer_applications.go

tests/integration/application/application_transfer_test.go

testutil/integration/suites/application.go

…ansfer-period * pokt/main: [Tooling] chore: re-organize params.mk targets (#833)

tests/integration/application/application_transfer_test.go

red-0ne

Almost there!

Left some NITs and a suggestion about undelegations merging.

x/application/keeper/transfer_applications.go

red-0ne · 2024-09-27T07:31:29Z

testutil/integration/suites/application.go

+}
+
+// Transfer sends a MsgApplicationTransfer to begin an application transfer
+// from srcBech32 to dstDst32.


Suggested change

// from srcBech32 to dstDst32.

// from srcBech32 to dstBech32.

I'm not a big fan of the bech32 term.

My intention was to disambiguate between cosmostypes.AccAddress and the string representation. I agree that it's not the prettiest and gets monotonous more quickly than addr. Do you think bech is to far removed to be clear or have a better idea?

tests/integration/application/application_transfer_test.go

x/application/keeper/transfer_applications.go

Co-authored-by: red-0ne <[email protected]>

…ansfer-period * pokt/main: [Gateway] chore: add `MsgUpdateParam` to gateway module (#808)

Olshansk

@bryanchriswhite Approving since I won't be available and overall it LGTM.

My biggest concerns / asks for the future:

Think of ways to make sure we use the test helpers you introduced
Add SIMPLE (< 7 elements per )mermaid diagrams in the docs showing what merging does and how it works

e2e/tests/init_test.go

Olshansk · 2024-10-02T20:20:42Z

x/application/keeper/transfer_applications.go

+
+	// Only process application transfers at the end of the session in
+	// order to avoid inconsistent/unpredictable mid-session behavior.
+	if !shared.IsSessionEndHeight(&sharedParams, currentHeight) {


Add a debug log please that includes current height and the next session end height

I would like to push back on this. This would produce a log for every block that's not a session end height. EndBlockerUnbondApplications() and EndBlockerUnbondSuppliers() both have a simliar conditional branch and neither includes the log that you're suggesting here.

Can you elaborate on the rationale or concern?

I've added the current and session end height to the logger output:

currentHeight := sdkCtx.BlockHeight() sharedParams := k.sharedKeeper.GetParams(ctx) sessionEndHeight := shared.GetSessionEndHeight(&sharedParams, currentHeight) logger := k.Logger(). With("method", "EndBlockerTransferApplication"). With("current_height", currentHeight). With("session_end_height", sessionEndHeight)

Olshansk · 2024-10-02T20:40:30Z

x/application/keeper/transfer_applications.go

+	}
+
+	// Iterate over all applications and transfer the ones that have finished the transfer period.
+	// TODO_MAINNET: Use an index to iterate over the applications that have initiated


Can you add your name here?

Would love it if you can start looking into indexing on-chain!

I've opened #854.

x/application/keeper/transfer_applications.go

x/application/keeper/msg_server_transfer_application.go

x/application/keeper/msg_server_unstake_application.go

Olshansk · 2024-10-02T20:45:15Z

x/application/keeper/transfer_applications.go

+
+// MergeAppServiceConfigs takes the union of the srcApp and dstApp's service configs
+// and sets the result in dstApp. It is exported for testing purposes.
+func MergeAppServiceConfigs(srcApp, dstApp *types.Application) {


Do you have a followup PR that updates the docs on this?

Would love a simple excalidraw/mermaid diagram for it.

I will include it in #790. 👍

x/application/keeper/transfer_applications.go

red-0ne

LGTM! 🥳

Co-authored-by: Daniel Olshansky <[email protected]>

…ake-gateway * pokt/main: [Application] chore: implement app transfer period (#789)

…e-param * issues/612/gateway/logic: [Application] chore: implement app transfer period (#789) chore: review feedback improvements chore: improve logging & ensure gRPC status error returns chore: add godoc style comment to min_stake params field chore: apply improvements Empty commit Empty commit

…ation/min-stake-param * issues/612/app/msg-update-param: [Application] chore: implement app transfer period (#789) chore: review feedback improvements chore: improve logging & ensure gRPC status error returns chore: add godoc style comment to min_stake params field chore: apply improvements Empty commit Empty commit # Conflicts: # api/poktroll/application/tx.pulsar.go # x/application/types/errors.go # x/application/types/tx.pb.go

…/application/staking * issues/612/application/min-stake-param: [Application] chore: implement app transfer period (#789) chore: review feedback improvements chore: improve logging & ensure gRPC status error returns chore: improve logging & ensure gRPC status error returns chore: improvements: chore: add godoc style comment to min_stake params field chore: add godoc style comment to min_stake params field chore: apply improvements Empty commit Empty commit # Conflicts: # x/application/keeper/msg_server_transfer_application_stake_test.go

…tion/burning * issues/612/application/staking: test: simplify coin equality assertions [Application] chore: implement app transfer period (#789) chore: review feedback improvements chore: improve logging & ensure gRPC status error returns chore: improve logging & ensure gRPC status error returns chore: improvements: chore: add godoc style comment to min_stake params field chore: add godoc style comment to min_stake params field chore: apply improvements Empty commit Empty commit # Conflicts: # x/application/keeper/unbond_applications.go

bryanchriswhite and others added 27 commits August 21, 2024 22:21

ignite scaffold message transfer-application-stake address:string ben…

c9945ad

…eficiary:string --module application

feat: implement application stake transfer

69ee2cb

resolve: merge conflicts

ce6428e

chore: revert whitespace changes

fc60584

chore: add address string protobuf options

49b7592

test: improve unit tests

8046491

chore: add telemetry

c23ca86

fix: MsgTransferApplicationStake#ValidateBasic()

ca95a92

chore: linting

8ac6935

test: add makefile target for e2e app transfer

27bb042

chore: update/lint app transfer autocli

43c7f85

test: E2E app stake transfer

7fd1bee

chore: remove TODO

fa6a63b

fix: e2e build constraint

1656787

chore: linting

0d99a89

chore: review feedback improvements

56d7f11

Co-authored-by: Daniel Olshansky <[email protected]>

rename: app_stake_transfer to stake_app_transfer

cb44753

chore: update test_e2e_app_transfer make target

76da405

chore: review feedback improvements

e9dee27

Merge remote-tracking branch 'pokt/main' into issues/657/feat/app-sta…

7fe7a0e

…ke-transfer * pokt/main: [Application] Implement unbonding period (#735) [Docs] Move over docs from poktroll-docker-compose-example (#757) [Performance] Reduce RelayMiner memory consumption under load (#739)

chore: post-merge improvements

e84f003

fix: error duplicate code

ea967be

fix: #ValidateBasic

5de7ddf

fix: keeper test

451ef64

Merge branch 'main' into issues/657/feat/app-stake-transfer

2f43521

refactor: rename TransferApplicationStake to TransferApplication

1cd9402

refactor: rename param to application_transfer_and_unboding_period_se…

9107be4

…ssions

bryanchriswhite added application Changes related to the Application actor on-chain On-chain business logic labels Sep 4, 2024

bryanchriswhite added this to the Shannon Beta TestNet Launch milestone Sep 4, 2024

github-actions bot reviewed Sep 25, 2024

View reviewed changes

bryanchriswhite added 3 commits September 25, 2024 17:49

chore: integration test improvements

325adf0

chore: integrate ParamsSuite

4fc7a97

fix: suite bugs

c0faf4b

github-actions bot reviewed Sep 25, 2024

View reviewed changes

bryanchriswhite added 2 commits September 25, 2024 18:12

chore: cleanup comments

1d7dac3

Merge remote-tracking branch 'pokt/main' into issues/657/chore/app-tr…

6e735e9

…ansfer-period * pokt/main: [Tooling] chore: re-organize params.mk targets (#833)

bryanchriswhite commented Sep 25, 2024

View reviewed changes

tests/integration/application/application_transfer_test.go Outdated Show resolved Hide resolved

bryanchriswhite added 3 commits September 25, 2024 19:05

fix: refactoring missing args

989b3c2

tests: rename & unexport test suites

d05cb99

Empty commit

fba58f2

red-0ne reviewed Sep 27, 2024

View reviewed changes

bryanchriswhite and others added 4 commits September 30, 2024 11:02

chore: review feedback improvement

691fed3

Co-authored-by: red-0ne <[email protected]>

chore: update comment

ef505e4

Merge branch 'main' into issues/657/chore/app-transfer-period

9857eaa

Empty commit

242b10b

bryanchriswhite requested a review from red-0ne September 30, 2024 10:56

bryanchriswhite added 2 commits September 30, 2024 12:58

Merge remote-tracking branch 'pokt/main' into issues/657/chore/app-tr…

5945ba4

…ansfer-period * pokt/main: [Gateway] chore: add `MsgUpdateParam` to gateway module (#808)

Empty commit

eba1332

Olshansk approved these changes Oct 2, 2024

View reviewed changes

red-0ne approved these changes Oct 3, 2024

View reviewed changes

bryanchriswhite mentioned this pull request Oct 4, 2024

[On-Cain] Add missing on-chain indices #854

Open

6 tasks

chore: review feedback improvements

76537e7

Co-authored-by: Daniel Olshansky <[email protected]>

bryanchriswhite merged commit 600ba59 into main Oct 4, 2024
13 checks passed

bryanchriswhite deleted the issues/657/chore/app-transfer-period branch October 4, 2024 09:20

bryanchriswhite added a commit that referenced this pull request Oct 4, 2024

Merge remote-tracking branch 'pokt/main' into issues/612/param/min-st…

0d36e0f

…ake-gateway * pokt/main: [Application] chore: implement app transfer period (#789)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Application] chore: implement app transfer period #789

[Application] chore: implement app transfer period #789

bryanchriswhite commented Sep 4, 2024 •

edited

Loading

red-0ne left a comment

red-0ne Sep 27, 2024

bryanchriswhite Sep 30, 2024

Olshansk left a comment

Olshansk Oct 2, 2024

bryanchriswhite Oct 4, 2024 •

edited

Loading

bryanchriswhite Oct 4, 2024

Olshansk Oct 2, 2024

bryanchriswhite Oct 4, 2024 •

edited

Loading

Olshansk Oct 2, 2024

bryanchriswhite Oct 4, 2024

red-0ne left a comment

	// from srcBech32 to dstDst32.
	// from srcBech32 to dstBech32.

[Application] chore: implement app transfer period #789

[Application] chore: implement app transfer period #789

Conversation

bryanchriswhite commented Sep 4, 2024 • edited Loading

Summary

Depends on

Issue

Type of change

Testing

Sanity Checklist

red-0ne left a comment

Choose a reason for hiding this comment

red-0ne Sep 27, 2024

Choose a reason for hiding this comment

bryanchriswhite Sep 30, 2024

Choose a reason for hiding this comment

Olshansk left a comment

Choose a reason for hiding this comment

Olshansk Oct 2, 2024

Choose a reason for hiding this comment

bryanchriswhite Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

bryanchriswhite Oct 4, 2024

Choose a reason for hiding this comment

Olshansk Oct 2, 2024

Choose a reason for hiding this comment

bryanchriswhite Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Olshansk Oct 2, 2024

Choose a reason for hiding this comment

bryanchriswhite Oct 4, 2024

Choose a reason for hiding this comment

red-0ne left a comment

Choose a reason for hiding this comment

bryanchriswhite commented Sep 4, 2024 •

edited

Loading

bryanchriswhite Oct 4, 2024 •

edited

Loading

bryanchriswhite Oct 4, 2024 •

edited

Loading