Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Implements require_transactional#7261

Merged
bkchr merged 12 commits intoparitytech:masterfrom
laminar-protocol:require_transactional
Oct 12, 2020
Merged

Implements require_transactional#7261
bkchr merged 12 commits intoparitytech:masterfrom
laminar-protocol:require_transactional

Conversation

@xlc
Copy link
Contributor

@xlc xlc commented Oct 5, 2020

Closes #7004

This introduces require_transaction and #[require_transactional], which under release build is nop, so for document purpose only.

Under debug build, it will check if it is been called under with_transaction and panic if not. This is useful to catch unintended usage of methods that are unsafe to be called without with_transaction.

TODOs:

  • Docs
  • Check if the cfg usage is correct (@bkchr can you help?)

@xlc xlc force-pushed the require_transactional branch from c245ee5 to 3aa19de Compare October 5, 2020 04:43
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Some comments and docs are also missing

xlc and others added 2 commits October 7, 2020 10:49
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Sorry for being soo slow.

I think these are the last required changes, after that we can merge it.


#[test]
#[should_panic(expected = "Require transaction not called within with_transaction")]
fn require_transactional_panic() {
Copy link
Member

Choose a reason for hiding this comment

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

This test will not panic on a local cargo test --all --release and that is bad.

I think the solution would be that we disable the test if debug_assertions are not enabled? Or do you got a better idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test cfg check won't work for this so I think all we can do is disable it when debug_assertions is not enabled.

Copy link
Member

Choose a reason for hiding this comment

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

In the end is that test here doing the same as the one from frame-support and we could just remove it.

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Some last nitpicks + merge master to make tests works.

After that it is okay.


#[test]
#[should_panic(expected = "Require transaction not called within with_transaction")]
fn require_transactional_panic() {
Copy link
Member

Choose a reason for hiding this comment

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

In the end is that test here doing the same as the one from frame-support and we could just remove it.

@bkchr bkchr added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Oct 11, 2020
@bkchr bkchr merged commit a230212 into paritytech:master Oct 12, 2020
@xlc xlc deleted the require_transactional branch October 12, 2020 07:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

#[require_transactional] annotation

2 participants