feat: consensus messages #351
Conversation
app/app.go
Outdated
|
|
||
| theType, err := proto.Marshal(resp) | ||
| if err != nil { | ||
| return nil |
There was a problem hiding this comment.
why return nil? don't you need to append to responses and continue?
There was a problem hiding this comment.
not sure about this, if the message passed but the resp is not correct?
There was a problem hiding this comment.
related to my previous question - should we maybe run it with cached context and if the message changed the state but the response is incorrect than simply uncommit the context? what happens in the same case if a tx runs? (i.e the response can't be marshalled?)
There was a problem hiding this comment.
I changed this code, but it will be a weird case when the message passes, but the response is not correct.
It looks like there will be a bug, and we end reverting the full tx.
| _, isError := response.Response.(*abci.ConsensusMessageResponse_Error) | ||
| require.True(t, isError, "Expected an error response but got a success") |
There was a problem hiding this comment.
i'd also suggest verifying the returned error string
There was a problem hiding this comment.
@danwt did not like to check the strings of errors
app/app.go
Outdated
| if err != nil { | ||
| responses = append(responses, &abci.ConsensusMessageResponse{ | ||
| Response: &abci.ConsensusMessageResponse_Error{ | ||
| Error: fmt.Errorf("failed to execute consensus message: %w", err).Error(), |
There was a problem hiding this comment.
app/app.go
Outdated
| continue | ||
| } | ||
|
|
||
| resp, err := app.MsgServiceRouter().Handler(msg)(ctx, msg) |
There was a problem hiding this comment.
shouldn't we here pass cache ctx and than abort in case of errors?
deliver tx provides it for us automatically but afair in begin block we need to handle it manually.
i.e - if the handler fails, how does the state changes get reverted?
There was a problem hiding this comment.
totally, I think the abort in case of errors is not what was expected. If there is an error the context is not written and we move to next message. Only the error message is appended in the response. Check the writeCache() that is done at the end of the loop. Only if everything is perfect it is written to state.
app/app.go
Outdated
|
|
||
| theType, err := proto.Marshal(resp) | ||
| if err != nil { | ||
| return nil |
There was a problem hiding this comment.
related to my previous question - should we maybe run it with cached context and if the message changed the state but the response is incorrect than simply uncommit the context? what happens in the same case if a tx runs? (i.e the response can't be marshalled?)
…xyz/rollapp-evm into feat/consensus-messages
Description
ADR link https://www.notion.so/dymension/ADR-x-Sequencer-Messages-34f95e2b579e458e8bb4252da19324bd
Closes https://github.com/dymensionxyz/epics/issues/1
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.
PR review checkboxes:
I have...
Unreleasedsection inCHANGELOG.mdgodoccommentsSDK Checklist
maptime.Now()sendCoinand notSendCoinsFull security checklist here
----;
For Reviewer:
---;
After reviewer approval: