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

Methods to add certificate scripts for smart staking #388

Merged
merged 8 commits into from
Oct 22, 2024

Conversation

SCMusson
Copy link
Contributor

Following on from #387 and #378, this should add support for staking with staking scripts.

Copy link

codecov bot commented Oct 14, 2024

Codecov Report

Attention: Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.76%. Comparing base (3034cd9) to head (c95fb61).
Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
pycardano/txbuilder.py 94.44% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #388      +/-   ##
==========================================
+ Coverage   85.30%   85.76%   +0.46%     
==========================================
  Files          32       32              
  Lines        4213     4202      -11     
  Branches     1060     1052       -8     
==========================================
+ Hits         3594     3604      +10     
+ Misses        434      421      -13     
+ Partials      185      177       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nielstron
Copy link
Contributor

There are some QA issues, make sure to run make qa and fix the remaining errors :)

poetry run flake8 pycardano
pycardano/txbuilder.py:161:1: W293 blank line contains whitespace
pycardano/txbuilder.py:399:121: E501 line too long (131 > 120 characters)
pycardano/txbuilder.py:416:121: E501 line too long (196 > 120 characters)
pycardano/txbuilder.py:519:1: W293 blank line contains whitespace
make: *** [Makefile:64: qa] Error 1

@SCMusson
Copy link
Contributor Author

Apologies, I should have read the development section more thoroughly.

@nielstron
Copy link
Contributor

No worries, I did the exact same in my first (few) PRs

@nielstron
Copy link
Contributor

We seem to have run into a docker limitation with the CI. @cffls do you know anything about this? Maybe just need to wait a day and retry

@SCMusson
Copy link
Contributor Author

Coverage should be 100% for the changes made now if someone wants to approve the CI

@@ -879,6 +930,8 @@ def _dfs(script: NativeScript):
def _set_redeemer_index(self):
# Set redeemers' index according to section 4.1 in
# https://hydra.iohk.io/build/13099856/download/1/alonzo-changes.pdf
#
# There is no way to determine certificate index here
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I understand correctly that the redeemer index is supposed to be directly set by the user according to the order in which they add certificates? (this is the way that it is currently implemented here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's how cardano-cli does it. I was thinking that if this causes confusion then the best alternative would be to create an add_certificate method with optional arguments to add a script and redeemer. I assumed that would be a more major change that might be out of scope for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are there script-less certificates?
Either way we can add this upon request

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The majority of delegation seems to be done with certificates built from stake keys rather than scripts. I think smart staking is relatively niche at the moment.

@nielstron
Copy link
Contributor

Only thing that is missing would be a test that actually submits a transaction that requires spending a SC certificate. Given I neither added this in #308 and no one has complained that it doesn't work I would proceed and approve this.
If @SCMusson you already have a simple reproducible instance of script+transactions that would validate against the spun-up ogmios instance in the CI here, feel free to add it, otherwise I think we can defer it to a follow-up PR.

@SCMusson
Copy link
Contributor Author

I presume you are referring to the tests in integration-test/test?

@nielstron
Copy link
Contributor

Yes

@SCMusson
Copy link
Contributor Author

I can't find a way to test withdrawing from a stake script based delegation as you get this error when trying to withdraw 0 ada:

ResponseError: Ogmios responded with error: {'jsonrpc': '2.0', 'method': 'submitTransaction', 'error': {'code': 3141, 'message': "The transaction contains incomplete or invalid rewards withdrawals. When present, rewards withdrawals must consume rewards in full, there cannot be any leftover. The field 'data.incompleteWithdrawals' contains a map of withdrawals and their current rewards balance.", 'data': {'incompleteWithdrawals': {'stake_test17rv0fzzfqdhuzvxtjdx6535l4qx0pwd9n7tp8kyr52gfwpq7eae2v': {'ada': {'lovelace': 0}}}}}, 'id': None}

Obviously there isn't time to allow rewards to accrue during a test.

@nielstron
Copy link
Contributor

Ok. This sounds funny since a major hack of saving memory in recent SC development was using 0 withdrawals - but maybe this refers to another script type.
I'll proceed and merge this, we can check later on how to test it.

@nielstron nielstron merged commit a2a9014 into Python-Cardano:main Oct 22, 2024
11 checks passed
@nielstron
Copy link
Contributor

Now you are also not a first-time contributor anymore and new PRs should have the CI run automatically :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants