Skip to content

[feature] #3525: Standardize transaction API#3485

Merged
mversic merged 1 commit intohyperledger-iroha:iroha2-devfrom
mversic:transaction_refactor
Jun 9, 2023
Merged

[feature] #3525: Standardize transaction API#3485
mversic merged 1 commit intohyperledger-iroha:iroha2-devfrom
mversic:transaction_refactor

Conversation

@mversic
Copy link
Copy Markdown
Contributor

@mversic mversic commented May 17, 2023

Description

Rules:

  • Never use structs named *Box in the client code or integration tests.
    These structs exist only for serialization. If you write this type explicitly, this is a smell and user facing API should be updated.
  • Never refer to V1 in any crate except data_model or in tests in other crates.
    Ideally, any versioned struct(I mean SignedTransaction should be private, not VersionedSignedTransaction) should not be accessible from outside code. This isn't possible atm because of some tests. We want the code in core/client to be version agnostic
  • there should be only one tranasction/block type in the data_model and it should be the 1st in the lifecycle
    except for the first, all other state transitions are internal and must not be exposed through data_model. This also means that VersionedCommittedBlock should be replaced with VersionedSignedBlock. This will be addressed in the next PR

Linked issue

Closes #3525

Benefits

Checklist

  • I've read CONTRIBUTING.md
  • I've used the standard signed-off commit format (or will squash just before merging)
  • All applicable CI checks pass (or I promised to make them pass later)
  • (optional) I've written unit tests for the code changes
  • I replied to all comments after code review, marking all implemented changes with thumbs up

@github-actions github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label May 17, 2023
@mversic mversic force-pushed the transaction_refactor branch 13 times, most recently from 577ade0 to 2c54e2f Compare May 19, 2023 06:07
@mversic mversic marked this pull request as ready for review May 19, 2023 06:10
@mversic mversic force-pushed the transaction_refactor branch 5 times, most recently from 151bb56 to 77f16b8 Compare May 19, 2023 10:11
@mversic mversic changed the title [refactor]: Standardize transaction API [feature] #3525: Standardize transaction API May 24, 2023
@mversic mversic force-pushed the transaction_refactor branch 2 times, most recently from 74bacc3 to e97bd46 Compare May 25, 2023 13:29
Comment thread cli/src/main.rs Outdated
Comment thread cli/src/lib.rs
@mversic mversic force-pushed the transaction_refactor branch 3 times, most recently from d54bdfd to d4567e2 Compare May 26, 2023 07:53
Comment thread client/tests/integration/permissions.rs Outdated
Comment thread configs/client/config.json Outdated
Comment thread core/src/block.rs
Comment thread config/kita Outdated
@mversic mversic force-pushed the transaction_refactor branch 2 times, most recently from d977393 to c34d62c Compare May 26, 2023 08:26
Comment thread core/src/tx.rs
Comment thread schema/gen/src/lib.rs Outdated
Comment thread data_model/src/transaction.rs
Comment thread data_model/src/transaction.rs
@coveralls
Copy link
Copy Markdown

coveralls commented May 30, 2023

Pull Request Test Coverage Report for Build 5122726761

  • 541 of 911 (59.39%) changed or added relevant lines in 53 files are covered.
  • 176 unchanged lines in 17 files lost coverage.
  • Overall coverage increased (+0.03%) to 57.925%

Changes Missing Coverage Covered Lines Changed/Added Lines %
core/src/smartcontracts/isi/block.rs 1 2 50.0%
core/src/smartcontracts/isi/world.rs 0 1 0.0%
core/src/smartcontracts/mod.rs 0 1 0.0%
data_model/src/evaluate.rs 0 1 0.0%
data_model/src/events/data/events.rs 5 6 83.33%
data_model/src/events/mod.rs 2 3 66.67%
data_model/src/peer.rs 0 1 0.0%
data_model/src/visit.rs 0 1 0.0%
cli/src/torii/mod.rs 0 2 0.0%
core/src/smartcontracts/isi/tx.rs 0 2 0.0%
Files with Coverage Reduction New Missed Lines %
core/src/block.rs 1 57.37%
core/src/gossiper.rs 1 0%
crypto/src/hash.rs 1 58.33%
data_model/src/events/pipeline.rs 1 78.4%
data_model/src/ipfs.rs 1 91.75%
data_model/src/lib.rs 1 50.82%
data_model/src/query.rs 1 20.72%
data_model/src/trigger.rs 1 43.62%
core/test_network/src/lib.rs 2 0.18%
data_model/src/block.rs 2 45.59%
Totals Coverage Status
Change from base Build 5109908822: 0.03%
Covered Lines: 18232
Relevant Lines: 31475

💛 - Coveralls

Copy link
Copy Markdown
Contributor

@0x009922 0x009922 left a comment

Choose a reason for hiding this comment

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

LGTM, although I don't understand most of the changes.

Comment thread config/src/client.rs Outdated
Comment thread core/src/queue.rs Outdated
Erigara
Erigara previously approved these changes May 31, 2023
Copy link
Copy Markdown
Contributor

@Erigara Erigara left a comment

Choose a reason for hiding this comment

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

LGTM, couple of small comments

Comment thread cli/src/main.rs Outdated
Comment thread data_model/src/block.rs
Erigara
Erigara previously approved these changes Jun 1, 2023
Comment thread docs/source/references/schema.json
Comment thread cli/src/torii/routing.rs
Comment thread cli/src/torii/routing.rs
Comment thread cli/src/torii/routing.rs
Comment thread client/tests/integration/asset.rs
Comment thread wasm/validator/src/permission.rs
Comment thread schema/src/lib.rs Outdated
Comment thread client/tests/integration/queries/asset.rs
Comment thread client/tests/integration/transfer_asset.rs
Comment thread client/tests/integration/tx_history.rs Outdated
Comment thread config/src/client.rs Outdated
Comment thread config/src/queue.rs Outdated
Comment thread configs/client/config.json Outdated
Comment thread data_model/src/isi.rs
Erigara
Erigara previously approved these changes Jun 6, 2023
Signed-off-by: Marin Veršić <marin.versic101@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

iroha2-dev The re-implementation of a BFT hyperledger in RUST

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants