Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Morse->Shannon Migration] scaffold: MorseAccountState single #1045

Open
wants to merge 8 commits into
base: chore/migration/state-prep
Choose a base branch
from

Conversation

bryanchriswhite
Copy link
Contributor

@bryanchriswhite bryanchriswhite commented Jan 29, 2025

Summary

Scaffolds the on-chain MorseAccountState single as well as a create message/query handlers. Supersedes #1035.

Changes:

  • Scaffolds the on-chain data structure and KV store getters/setters.
  • Scaffolds a create message and query handlers for uploading the morse account state.

Issue

Type of change

Select one or more from the following:

Sanity Checklist

  • I have updated the GitHub Issue assignees, reviewers, labels, project, iteration and milestone
  • For docs, I have run make docusaurus_start
  • For code, I have run make go_develop_and_test and make test_e2e
  • For code, I have added the devnet-test-e2e label to run E2E tests in CI
  • For configurations, I have update the documentation
  • I added TODOs where applicable

@bryanchriswhite bryanchriswhite added on-chain On-chain business logic consensus-breaking IMPORTANT! If the PR with this tag is merged, next release WILL HAVE TO BE an upgrade. labels Jan 29, 2025
@bryanchriswhite bryanchriswhite self-assigned this Jan 29, 2025
@bryanchriswhite bryanchriswhite force-pushed the issues/1034/scaffold/morse_account_state branch from 89b83f7 to b720680 Compare January 29, 2025 12:58
@bryanchriswhite bryanchriswhite linked an issue Jan 29, 2025 that may be closed by this pull request
9 tasks
@bryanchriswhite bryanchriswhite force-pushed the issues/1034/scaffold/morse_account_state branch from b720680 to cf4a0c5 Compare January 29, 2025 13:16
@bryanchriswhite bryanchriswhite marked this pull request as ready for review January 29, 2025 13:54
@Olshansk Olshansk self-requested a review January 29, 2025 18:30
@Olshansk Olshansk added the migration Morse to Shannon migration related work label Jan 29, 2025
proto/poktroll/migration/query.proto Outdated Show resolved Hide resolved
proto/poktroll/migration/query.proto Show resolved Hide resolved
proto/poktroll/migration/tx.proto Show resolved Hide resolved
proto/poktroll/migration/tx.proto Show resolved Hide resolved
proto/poktroll/migration/tx.proto Show resolved Hide resolved

val, found := k.GetMorseAccountState(ctx)
if !found {
return nil, status.Error(codes.NotFound, "not found")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do some logging here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To what end, can you elaborate?

@@ -0,0 +1,48 @@
package keeper_test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: I'm skiping reviewing unit tests in leu of time.

}
}

func (msg *MsgCreateMorseAccountState) ValidateBasic() error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not need to validate morseAccountState?

Copy link
Contributor Author

@bryanchriswhite bryanchriswhite Feb 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you have something specific in mind? I've add a hash comparison in #ValidateBasic(), but I'm reluctant to add logic that requires iterating over all accounts for similar reasons as mentioned elsewhere (performance). This seems redundant to me regardless, so long as we intend to rely on off-chain verification/consensus.

We should be able to assume the validity because it's generated from the new migrate sub-command and should be deterministically reproducible and should have social consensus prior to being created on-chain in Shannon.

message QueryGetMorseAccountStateRequest {}

message QueryGetMorseAccountStateResponse {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E2E test for this? Doesn't have to be in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have unit/integration coverage over this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus-breaking IMPORTANT! If the PR with this tag is merged, next release WILL HAVE TO BE an upgrade. migration Morse to Shannon migration related work on-chain On-chain business logic
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

[Morse->Shannon Migration] Migration module
2 participants