[Morse->Shannon Migration] feat: implement Morse account state upload #1047
Conversation
bryanchriswhite
added
on-chain
bryanchriswhite
force-pushed
the
issues/1034/feat/upload-state
branch
2 times, most recently
from
7b536e8 to
b90f019
Compare
bryanchriswhite
force-pushed
the
issues/1034/feat/upload-state
branch
from
b90f019 to
0180b2f
Compare
bryanchriswhite
force-pushed
the
issues/1034/feat/upload-state
branch
from
74601cc to
1add213
Compare
| "github.com/cosmos/gogoproto/proto" | ||
| ) | ||
|
|
||
| func (m MorseAccountState) GetHash() ([]byte, error) { |
There was a problem hiding this comment.
#PUC
- That it uses sha256
- It is not a "blockchain state hash" but a hash of the structure. Important destinction
| import "poktroll/shared/service.proto"; | ||
| import "poktroll/migration/types.proto"; | ||
|
|
||
| // EventUploadMorseState is emitted when a new state hash is uploaded. |
There was a problem hiding this comment.
- EventUploadMorseState != EventCreateMorseAccountState
state hashor the whole thing?
|
|
||
| // EventUploadMorseState is emitted when a new state hash is uploaded. | ||
| message EventCreateMorseAccountState { | ||
| int64 height = 1 [(gogoproto.jsontag) = "height"]; |
There was a problem hiding this comment.
Is this a snapshot height or the height of morse?
| func (k msgServer) CreateMorseAccountState(goCtx context.Context, msg *types.MsgCreateMorseAccountState) (*types.MsgCreateMorseAccountStateResponse, error) { | ||
| ctx := sdk.UnwrapSDKContext(goCtx) | ||
| func (k msgServer) CreateMorseAccountState(ctx context.Context, msg *types.MsgCreateMorseAccountState) (*types.MsgCreateMorseAccountStateResponse, error) { | ||
| sdkCtx := sdk.UnwrapSDKContext(ctx) |
| return nil, errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "already set") | ||
| return nil, status.Error( | ||
| codes.FailedPrecondition, | ||
| sdkerrors.ErrInvalidRequest.Wrap("already set").Error(), |
There was a problem hiding this comment.
Not a TODO, but just an FYI that we may need to handle updates.
No one knows what the requirements of that will look like yet.
There was a problem hiding this comment.
I think we SHOULD NOT. This is a very slippery slope. My expectation is that we do one or more "dress rehearsals" on Shannon testnet, using real Morse migration state, prior to doing it on Shannon mainnet.
Supporting updates to the imported account state necessarily adds very noteworthy complexity that I don't think really gives us any value in return.
See: #1045 (comment)
| msg.MorseAccountState, | ||
| ) | ||
| return &types.MsgCreateMorseAccountStateResponse{}, nil | ||
|
|
||
| stateHash, err := msg.MorseAccountState.GetHash() |
There was a problem hiding this comment.
Confirming that this is the actual "Morse Blockchain State Hash" and not just a "hash of the snapshot we make"?
There was a problem hiding this comment.
Will #PUC, it is a hash of the unsigned MsgCreateMorseAccountState.
|
|
||
| if err = sdkCtx.EventManager().EmitTypedEvent( | ||
| &types.EventCreateMorseAccountState{ | ||
| Height: sdkCtx.BlockHeight(), |
There was a problem hiding this comment.
Could you add some more details in this event given it'll only ever fired once (or a handful depending on how things evolve)?
E.g. Total amount of upokt transferred, total amount of accounts, etc?
There was a problem hiding this comment.
It should be fairly trivial to add the number of accounts.
I was already reluctant to do the hash computation on-chain due to concern about the CPU required to do so as the serialized state may be very large (tens of MB). Iterating over ~50K+ accounts to sum their amounts may take some time. I chose to avoid going down that rabbit hole. Happy to get out my shovel and hardhat if you think it's necessary.
|
|
||
| "github.com/pokt-network/poktroll/x/migration/types" | ||
| ) | ||
|
|
||
| func (k msgServer) CreateMorseAccountState(goCtx context.Context, msg *types.MsgCreateMorseAccountState) (*types.MsgCreateMorseAccountStateResponse, error) { | ||
| ctx := sdk.UnwrapSDKContext(goCtx) | ||
| func (k msgServer) CreateMorseAccountState(ctx context.Context, msg *types.MsgCreateMorseAccountState) (*types.MsgCreateMorseAccountStateResponse, error) { |
There was a problem hiding this comment.
Consider multilining heaer for readability
| // NewMorseStateExportAndAccountStateBytes returns a serialized `MorseStateExport` | ||
| // and its corresponding `MorseAccountState`, populated dynamically with randomized | ||
| // account addresses, and monotonically increasing balances/stakes. For each account, | ||
| // one application and supplier are also added to the states. | ||
| // TODO_CONSIDERATION: Test/benchmark execution speed can be optimized by refactoring this to a pre-generate fixture. |
There was a problem hiding this comment.
If you use the "Code Cleaner" claude project I created, this is how it refactored the comments. Much easier to read.
| // NewMorseStateExportAndAccountStateBytes returns a serialized `MorseStateExport` | |
| // and its corresponding `MorseAccountState`, populated dynamically with randomized | |
| // account addresses, and monotonically increasing balances/stakes. For each account, | |
| // one application and supplier are also added to the states. | |
| // TODO_CONSIDERATION: Test/benchmark execution speed can be optimized by refactoring this to a pre-generate fixture. | |
| // NewMorseStateExportAndAccountStateBytes returns: | |
| // - A serialized MorseStateExport | |
| // - Its corresponding MorseAccountState | |
| // | |
| // The states are populated with: | |
| // - Random account addresses | |
| // - Monotonically increasing balances/stakes | |
| // - One application per account | |
| // - One supplier per account | |
| // | |
| // TODO_CONSIDERATION: Test/benchmark execution speed can be optimized by refactoring this to a pre-generate fixture. |
| // NewMorseStateExportAndAccountState returns a `MorseStateExport` and its | ||
| // corresponding `MorseAccountState`, populated dynamically with randomized | ||
| // account addresses, and monotonically increasing balances/stakes. For each account, | ||
| // one application and supplier are also added to the states. |
There was a problem hiding this comment.
| // NewMorseStateExportAndAccountState returns a `MorseStateExport` and its | |
| // corresponding `MorseAccountState`, populated dynamically with randomized | |
| // account addresses, and monotonically increasing balances/stakes. For each account, | |
| // one application and supplier are also added to the states. | |
| // NewMorseStateExportAndAccountState returns MorseStateExport and MorseAccountState | |
| // structs populated with: | |
| // - Random account addresses | |
| // - Monotonically increasing balances/stakes | |
| // - One application per account | |
| // - One supplier per account | |
| func NewMorseStateExportAndAccountState() (*types.MorseStateExport, *types.MorseAccountState) |
Summary
Implement Morse account state upload to Shannon.
Changes:
MsgCreateMorseAccountStatehandler.Issue
Type of change
Select one or more from the following:
consensus-breakinglabel if so. See [Infra] Automatically add theconsensus-breakinglabel #791 for detailsSanity Checklist
assignees,reviewers,labels,project,iterationandmilestonemake docusaurus_startmake go_develop_and_testandmake test_e2edevnet-test-e2elabel to run E2E tests in CI