Skip to content

Conversation

dmitry-timofeev
Copy link
Contributor

Overview

Updated how transaction result is represented. This PR uses an alternative format with an enum to represent the error kind, yet to be done in the core (ECR-3717).

Dropped the internal type representing the transaction result. It has both pros and cons:

  • (+) No need for an extra value type(s) and conversion code
  • (?) Directly serializable in protobuf
  • (-) Less-than-optimal interface and documentation of the generated code.
  • (-) Less freedom to hide any complexities and/or provide better interface.
  • (-) Worse JSON compatibility.

See: ECR-3693

Definition of Done

  • There are no TODOs left in the code
  • Change is covered by automated tests
  • The coding guidelines are followed
  • Public API has Javadoc
  • Method preconditions are checked and documented in the Javadoc of the method
  • Changelog is updated if needed (in case of notable or breaking changes)
  • The continuous integration build passes

WIP:
 - Review and update docs, if needed
 - Unexpected exception handling — submit Jira?
 - Migrate to correct ExecutionStatus and remove redundant .proto
 in core.
@dmitry-timofeev dmitry-timofeev added the work-in-progress 👷‍♂️ Do not expect reviewers to provide any feedback on WIP PRs — please ask for it explicitly! label Oct 22, 2019
return ImmutableList.of(
arguments(0, "Min error code"),
arguments(1, ""),
arguments(Integer.MAX_VALUE, "Max error code")
Copy link
Contributor

Choose a reason for hiding this comment

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

Did core remove 255 as maximum error code? Can't find such a restriction in native anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it is still there, as Service error variant accepts only u8 as a code.

# Conflicts:
#	exonum-java-binding/common/src/main/java/com/exonum/binding/common/blockchain/TransactionResult.java
#	exonum-java-binding/common/src/main/proto/runtime.proto
#	exonum-java-binding/core/src/main/proto/blockchain.proto
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.005%) to 86.059% when pulling 3b67e28 on dmitry-timofeev:fix-tx-result-ECR-3693 into ab0cc99 on exonum:dynamic-services.

@dmitry-timofeev dmitry-timofeev removed the work-in-progress 👷‍♂️ Do not expect reviewers to provide any feedback on WIP PRs — please ask for it explicitly! label Nov 5, 2019
@dmitry-timofeev dmitry-timofeev merged commit d2dfb3f into exonum:dynamic-services Nov 5, 2019
@dmitry-timofeev dmitry-timofeev deleted the fix-tx-result-ECR-3693 branch November 5, 2019 15:47
@MakarovS MakarovS mentioned this pull request Nov 8, 2019
2 tasks
dmitry-timofeev pushed a commit that referenced this pull request Nov 8, 2019
Enabled TestKit tests that depended on transaction
results update (#1174).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants