-
Notifications
You must be signed in to change notification settings - Fork 277
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
[feature] #2037: Introduce Pre-commit Triggers #2041
[feature] #2037: Introduce Pre-commit Triggers #2041
Conversation
Signed-off-by: Daniil Polyakov <[email protected]>
Signed-off-by: Daniil Polyakov <[email protected]>
Signed-off-by: Daniil Polyakov <[email protected]>
Codecov Report
@@ Coverage Diff @@
## iroha2-dev #2041 +/- ##
==============================================
+ Coverage 77.94% 77.98% +0.04%
==============================================
Files 176 176
Lines 23982 24039 +57
==============================================
+ Hits 18692 18747 +55
- Misses 5290 5292 +2
Continue to review full report at Codecov.
|
I'd like to comment that we could need pre-commit triggers for other purposes. For example for the atomic swap and offline transactions. |
IntoSchema, | ||
Hash, | ||
)] | ||
pub enum ExecutionTime { |
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.
This is an option-like enum. It should use the niche available in the other data type to encode the discriminant, but I would double check.
Otherwise flatten.
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.
Yep. The sizes are identical even in Debug builds.
IntoSchema, | ||
Hash, | ||
)] | ||
pub enum ExecutionTime { |
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.
Yep. The sizes are identical even in Debug builds.
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.
One question remains about its naming.
What is the reason we can say it is not post-commit but pre-commit?
It looks to me every-commit or per-commit
Description of the Change
Initially I was planning to implement 3'rd test-case from #1890, but I forgot, that me and @appetrosyan decided to implement it with roles, not with triggers. But in the moment of realizing I've already finished this task, so here I am
Issue
Closes #2037
Benefits
Pre-commit triggers!
Possible Drawbacks
None
Usage Examples or Tests
See
client/tests/integration/triggers/time_trigger.rs