-
-
Notifications
You must be signed in to change notification settings - Fork 76
Add functions to automatically add required signers and validity range #179
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
Conversation
cc @juliusfrost |
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #179 +/- ##
==========================================
- Coverage 86.38% 85.81% -0.58%
==========================================
Files 26 26
Lines 2761 2812 +51
Branches 654 672 +18
==========================================
+ Hits 2385 2413 +28
- Misses 284 295 +11
- Partials 92 104 +12
... and 2 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
8a2ca3c
to
d75c6b9
Compare
@cffls The only error was that codecov could not upload the results, could you re-run? Other than that, I addressed your changes, please re-review :) |
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.
Thank you! Looks good overall. Just one minor comment about the computation of required signers.
pycardano/txbuilder.py
Outdated
input_addresses = [ | ||
i.output.address for i in self.inputs + self.collaterals | ||
] + [ | ||
Address.from_primitive(a) if isinstance(a, str) else a | ||
for a in self.input_addresses | ||
] | ||
required_signers = set( | ||
a.payment_part | ||
for a in input_addresses | ||
if isinstance(a.payment_part, VerificationKeyHash) | ||
) |
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.
You can probably use _input_vkey_hashes
here.
pycardano/pycardano/txbuilder.py
Lines 661 to 666 in db29ee4
def _input_vkey_hashes(self) -> Set[VerificationKeyHash]: | |
results = set() | |
for i in self.inputs + self.collaterals: | |
if isinstance(i.output.address.payment_part, VerificationKeyHash): | |
results.add(i.output.address.payment_part) | |
return results |
I don't really understand why the continuous integration failed there, do you see it @cffls ? |
Integration test is non-deterministic. It will fail like 5% of time. Looks like retrying works. |
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.
LGTM. Thank you!
Validity range and required signers are required by plutus scripts.
Users don't expect having to specify them if they add the utxos from specific addresses. Also validity range could be assumed to be somewhere around tx creation.
This PR adds the (default enabled) option to the transaction builder to automatically set the validity range and the required signers of the transaction to a reasonable value, which reduces friction for getting started with simple contracts.