-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Remove ModifiedTxStatus
from the spec and the code
#8210
Remove ModifiedTxStatus
from the spec and the code
#8210
Conversation
This reverts commit e576708.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few fixups and language changes. Overall very happy with this change.
I think the loop flow of removing a value on the next iteration is a bit confusing and prone to bugs if maintainers in the future add more complex flow control to the loop, say breaking after a certain condition. I would re-work the loops that check if the Tx list is too large so that the loops only add the transaction to the list it is building if it allows the full list to stay under MaxBytes
.
* If it does: remove transactions at the end of the list until the total byte size conforms to the limit, | ||
then set `ResponsePrepareProposal.modified_tx_status` to `MODIFIED` and return. | ||
* Else, set `ResponsePrepareProposal.modified_tx_status` to `UNMODIFIED` and return. | ||
* `PrepareProposal` should create a list of [TxRecord](./abci%2B%2B_methods_002_draft.md#txrecord) each containing a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this link needs to be URL encoded. My browser appears to do it automatically, although, perhaps this isn't universal behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are URL encoded all over. I've changed this one following your suggestion. If it works, I'll do another pass and change the rest
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Looks like the linter is complaining about the code not beeing gofmt
-ed. Pretty common go practice is to setup your editor to automatically run gofmt
on save. Happy to help set this with you if you'd like.
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really happy how this turned out.
…8210) * Outstanding abci-gen changes to 'pb.go' files * Removed modified_tx_status from spec and protobufs * Fix sed for OSX * Regenerated abci protobufs with 'abci-proto-gen' * Code changes. UTs e2e tests passing * Recovered UT: TestPrepareProposalModifiedTxStatusFalse * Adapted UT * Fixed UT * Revert "Fix sed for OSX" This reverts commit e576708. * Update internal/state/execution_test.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update abci/example/kvstore/kvstore.go Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * Update internal/state/execution_test.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update spec/abci++/abci++_tmint_expected_behavior_002_draft.md Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Addressed some comments * Added one test that tests error at the ABCI client + Fixed some mock calls * Addressed remaining comments * Update abci/example/kvstore/kvstore.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update abci/example/kvstore/kvstore.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update abci/example/kvstore/kvstore.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update spec/abci++/abci++_tmint_expected_behavior_002_draft.md Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Addressed William's latest comments * Adressed Michael's comment * Fixed UT * Some md fixes * More md fixes * gofmt Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
* -----start------ * [cherrypicked] state: panic on ResponsePrepareProposal validation error (#8145) * state: panic on ResponsePrepareProposal validation error * lint++ Co-authored-by: Sam Kleinman <garen@tychoish.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * [cherrypicked] abci++: remove CheckTx call from PrepareProposal flow (#8176) * [cherrypicked] abci++: correct max-size check to only operate on added and unmodified (#8242) * [cherrypicked] Remove `ModifiedTxStatus` from the spec and the code (#8210) * Outstanding abci-gen changes to 'pb.go' files * Removed modified_tx_status from spec and protobufs * Fix sed for OSX * Regenerated abci protobufs with 'abci-proto-gen' * Code changes. UTs e2e tests passing * Recovered UT: TestPrepareProposalModifiedTxStatusFalse * Adapted UT * Fixed UT * Revert "Fix sed for OSX" This reverts commit e576708. * Update internal/state/execution_test.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update abci/example/kvstore/kvstore.go Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * Update internal/state/execution_test.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update spec/abci++/abci++_tmint_expected_behavior_002_draft.md Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Addressed some comments * Added one test that tests error at the ABCI client + Fixed some mock calls * Addressed remaining comments * Update abci/example/kvstore/kvstore.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update abci/example/kvstore/kvstore.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update abci/example/kvstore/kvstore.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update spec/abci++/abci++_tmint_expected_behavior_002_draft.md Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Addressed William's latest comments * Adressed Michael's comment * Fixed UT * Some md fixes * More md fixes * gofmt Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * make proto-gen * Fixed testcase on PrepareProposal error * mockery Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> Co-authored-by: Sam Kleinman <garen@tychoish.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
* -----start------ * [cherrypicked] state: panic on ResponsePrepareProposal validation error (#8145) * state: panic on ResponsePrepareProposal validation error * lint++ Co-authored-by: Sam Kleinman <garen@tychoish.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * [cherrypicked] abci++: remove CheckTx call from PrepareProposal flow (#8176) * [cherrypicked] abci++: correct max-size check to only operate on added and unmodified (#8242) * [cherrypicked] Remove `ModifiedTxStatus` from the spec and the code (#8210) * Outstanding abci-gen changes to 'pb.go' files * Removed modified_tx_status from spec and protobufs * Fix sed for OSX * Regenerated abci protobufs with 'abci-proto-gen' * Code changes. UTs e2e tests passing * Recovered UT: TestPrepareProposalModifiedTxStatusFalse * Adapted UT * Fixed UT * Revert "Fix sed for OSX" This reverts commit e576708. * Update internal/state/execution_test.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update abci/example/kvstore/kvstore.go Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * Update internal/state/execution_test.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update spec/abci++/abci++_tmint_expected_behavior_002_draft.md Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Addressed some comments * Added one test that tests error at the ABCI client + Fixed some mock calls * Addressed remaining comments * Update abci/example/kvstore/kvstore.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update abci/example/kvstore/kvstore.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update abci/example/kvstore/kvstore.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update spec/abci++/abci++_tmint_expected_behavior_002_draft.md Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Addressed William's latest comments * Adressed Michael's comment * Fixed UT * Some md fixes * More md fixes * gofmt Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * make proto-gen * Fixed testcase on PrepareProposal error * mockery Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> Co-authored-by: Sam Kleinman <garen@tychoish.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
ModifiedTxStatus
fromResponsePrepareProposal
as it might be misleading to set it toUNMODIFIED
when the total size of transactions passed to the App exceedsRequestPrepareProposal.MaxTxBytes
. In this case, the Applications is required to modify the transactions by shedding as many as needed to conform to the size limit.master
. As a result, runningmake abci-proto-gen
on top of this PR, does not produce any uncommitted changes on auto-generated code