Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[refactor]: Move TriggerSet to data_model #2229

Merged
merged 4 commits into from
May 20, 2022
Merged

[refactor]: Move TriggerSet to data_model #2229

merged 4 commits into from
May 20, 2022

Conversation

Arjentix
Copy link
Contributor

Description of the Change

Initially it suppose to close #1889 but in the middle of work it was decided to wait until we get dynamic wasm linking. Dynamic linking is important, cause it will remove no_std limitations from data_model and TriggerSet is not compatible with no_std.
So this PR contains only TriggerSet moving to data_model. It should be useful for the future.

Issue

None

Benefits

  • TriggerSet now is stored there it should be
  • Errors produced by TriggerSet make more sense

Possible Drawbacks

None

@Arjentix Arjentix added iroha2-dev The re-implementation of a BFT hyperledger in RUST Refactor Improvement to overall code quality labels May 18, 2022
@Arjentix Arjentix self-assigned this May 18, 2022
@codecov
Copy link

codecov bot commented May 18, 2022 •
edited
Loading

Codecov Report

Merging #2229 (4257195) into iroha2-dev (8374d8b) will decrease coverage by 76.46%.
The diff coverage is n/a.

❗ Current head 4257195 differs from pull request most recent head 793b7d7. Consider uploading reports for the commit 793b7d7 to get more accurate results

@@              Coverage Diff               @@
##           iroha2-dev   #2229       +/-   ##
==============================================
- Coverage       76.46%       0   -76.47%     
==============================================
  Files             186       0      -186     
  Lines           26929       0    -26929     
==============================================
- Hits            20592       0    -20592     
+ Misses           6337       0     -6337
Impacted Files Coverage Δ
client/benches/tps/lib.rs
client/examples/million_accounts_tx.rs
client/examples/ten_million_accounts.rs
client/src/client.rs
client/src/http_default.rs
client/tests/integration/add_account.rs
client/tests/integration/add_asset.rs
client/tests/integration/add_domain.rs
client/tests/integration/asset_propagation.rs
client/tests/integration/connected_peers.rs
... and 176 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa67941...793b7d7. Read the comment docs.

@Arjentix Arjentix force-pushed the move_trigger_set_to_data_model branch 2 times, most recently from 7722290 to bcfa66c Compare May 19, 2022 12:37
appetrosyan
appetrosyan reviewed May 19, 2022
View reviewed changes
core/src/smartcontracts/isi/triggers.rs
wsv.modify_triggers(|triggers| {
triggers.remove(&trigger)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would think that removal is an idempotent instruction, but since we flag many other repetitions as errors, this is a welcome change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it was a Repetition error before, but it was done in TriggerSet. Now it is done here, because TriggerSet should know nothing about instructions

appetrosyan
appetrosyan reviewed May 19, 2022
View reviewed changes
core/src/smartcontracts/isi/triggers.rs

triggers.mod_repeats(&id, |n| {
n.checked_add(self.object).ok_or(MathError::Overflow)
n.checked_add(self.object)
.ok_or(trigger::set::RepeatsOverflowError)
Copy link
Contributor

Choose a reason for hiding this comment

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

More specific kind of error, should help readability.

appetrosyan
appetrosyan previously approved these changes May 19, 2022
View reviewed changes
@Arjentix Arjentix force-pushed the move_trigger_set_to_data_model branch from bcfa66c to 49382d7 Compare May 19, 2022 21:03
@appetrosyan appetrosyan force-pushed the move_trigger_set_to_data_model branch from 49382d7 to 4257195 Compare May 20, 2022 06:54
pesterev
pesterev approved these changes May 20, 2022
View reviewed changes
Copy link
Contributor

@pesterev pesterev left a comment

Choose a reason for hiding this comment

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

LGTM

@Arjentix @appetrosyan
Move TriggerSet from core to data_model
c75c420 
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
@Arjentix @appetrosyan
Make TriggerSet be compiled only in std build
c612bd2 
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
@Arjentix @appetrosyan
Make tokio and dashmap optional
75ea8ef 
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
@Arjentix @appetrosyan
Add more method links to doc comments
793b7d7 
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
@appetrosyan appetrosyan force-pushed the move_trigger_set_to_data_model branch from 4257195 to 793b7d7 Compare May 20, 2022 09:11
@appetrosyan appetrosyan merged commit e5c39e4 into hyperledger:iroha2-dev May 20, 2022
@Arjentix Arjentix deleted the move_trigger_set_to_data_model branch May 20, 2022 10:22
pesterev pushed a commit to pesterev/iroha that referenced this pull request May 25, 2022
@Arjentix @pesterev
[refactor]: Move TriggerSet to data_model (hyperledger#2229)
1697555 
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pesterev pesterev pesterev approved these changes

@appetrosyan appetrosyan appetrosyan approved these changes

@s8sato s8sato Awaiting requested review from s8sato

@mversic mversic Awaiting requested review from mversic

@SamHSmith SamHSmith Awaiting requested review from SamHSmith

@QuentinI QuentinI Awaiting requested review from QuentinI

@outoftardis outoftardis Awaiting requested review from outoftardis

Assignees

@Arjentix Arjentix

Labels
iroha2-dev The re-implementation of a BFT hyperledger in RUST Refactor Improvement to overall code quality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Arjentix @pesterev @appetrosyan