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.

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

feat: add expiration transaction policy #1583

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

hal3e
Copy link
Contributor

@hal3e hal3e commented Jan 21, 2025

closes: #1577

Release notes

Added the expiration transaction policy. Now the user can limit until which block height the transaction is valid.

Breaking Changes

  • added expiration: Option<u64> field to TxPolicies struct
  • Transaction trait method maturity now returns Option<u64>
  • Transaction trait method with_maturity is removed.

Checklist

  • All changes are covered by tests (or not applicable)
  • All changes are documented (or not applicable)
  • I reviewed the entire PR myself (preferably, on GH UI)
  • I described all Breaking Changes (or there's none)

@hal3e hal3e added the enhancement New feature or request label Jan 21, 2025
@hal3e hal3e self-assigned this Jan 21, 2025
digorithm
digorithm previously approved these changes Jan 23, 2025
Copy link
Member

@digorithm digorithm left a comment

Choose a reason for hiding this comment

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

Neat -- great tests!

MujkicA
MujkicA previously approved these changes Jan 24, 2025
Copy link
Contributor

@MujkicA MujkicA left a comment

Choose a reason for hiding this comment

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

Left a remark about our general approach to testing.
PR looks good :shipit:

@@ -248,6 +260,8 @@ pub trait Transaction:

fn with_maturity(self, maturity: u32) -> Self;

fn expiration(&self) -> Option<u64>;
Copy link
Contributor

Choose a reason for hiding this comment

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

should we a setter with_expiration here, like we have for maturity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. Well other policies are also missing the with_ methods. We can either remove it or add them for all? What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with_maturity changes the tx_id so I would like to remove it.

@@ -1245,3 +1245,80 @@ async fn predicate_configurables_in_blobs() -> Result<()> {

Ok(())
}

#[tokio::test]
async fn predicate_transfer_respects_maturity_and_expiration() -> Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

At some point, we should discuss how we want to approach testing for features like this, where we cover various transaction and input types. It feels like the level of granularity here (and in other cases) might be higher than what we actually benefit from.

@hal3e hal3e dismissed stale reviews from MujkicA and digorithm via 967e20b January 27, 2025 09:46
@hal3e hal3e requested review from digorithm and MujkicA January 27, 2025 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support new policies: Expiration and Owner
3 participants