Skip to content

Add add_native_script method to TransactionBuilder #255

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

Closed
wants to merge 1 commit into from

Conversation

cardano-apexpool
Copy link

When creating a transaction spending funds from a script address, there is no obvious way of adding the script input to the transaction.

One way is to manually select the input UTxOs and then add them to the transaction using:
builder.add_script_input(utxo, native_script)

But if you want to let PyCardano select tin input UTxOs from the script address:
builder.add_input_address(script_address)
then you must manually add the native script like this:
builder.native_scripts = [native_script]
but you need to check if builder.native_scripts is not already containing native scripts.

With this new method, it would me more straightforward:
builder.add_native_script(native_script)

@nielstron
Copy link
Contributor

nielstron commented Jun 19, 2023

What is the delta of this to builder.native_scripts.append(native_script)?

UPDATE: I just checked the diff and seems like native_scripts can be None. I think this is a good change. Does it make sense to check for uniqueness i.e. not add the script if it is contained in the list already?

@cardano-apexpool
Copy link
Author

I believe it makes sense to check for uniqueness. You can make changes to the code if you want, it can probably be improved. I just think that such a method would be good, I was looking for it when trying to create my transaction and I saw that there isn't such a method.

@codecov-commenter
Copy link

codecov-commenter commented Jun 19, 2023

Codecov Report

Merging #255 (089d350) into main (bc02bd6) will decrease coverage by 0.11%.
The diff coverage is 20.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #255      +/-   ##
==========================================
- Coverage   84.97%   84.86%   -0.11%     
==========================================
  Files          26       26              
  Lines        2982     2987       +5     
  Branches      714      715       +1     
==========================================
+ Hits         2534     2535       +1     
- Misses        336      340       +4     
  Partials      112      112              
Impacted Files Coverage Δ
pycardano/txbuilder.py 89.68% <20.00%> (-0.65%) ⬇️

@nielstron
Copy link
Contributor

I think before we merge this definitely also needs a test case.

On a seperate note I am wondering whether it makes sense to just add another parameter to "add_input_address" which lets you pass the native script. Or is there any other situation where you would want to plainly add a native script?

@cardano-apexpool
Copy link
Author

Adding another parameter to "add_input_address" for the native script is a very good idea. I am not aware of any situation where it would be required to add the script separately.

@cffls
Copy link
Collaborator

cffls commented Jun 20, 2023

Thanks for the change. Maybe it was not documented clearly, but you can also use add_minting_script to add a native script. The script will get added to _minting_script_to_redeemers, which will later get added to witness. The builder will also check the uniqueness of the scripts that were added through add_minting_script.

@cardano-apexpool
Copy link
Author

I tried using the add_minting_script method, but it is not working. This is the updated code:

transaction.add_minting_script(script=native_script)
# transaction.native_scripts = [native_script]

When building the transaction, I get the error:

ValueError: ScriptHash(hex='b5c0307c6e64460bd1b14d21769b55f1fe271caaeb230f4d8f17c5bd') is not in list

This does not happen when I am using the commented part in my code instead of the add_minting_script method (or when using the method I submitted in the pull request).

I am happy with any solution that works instead of my workaround (transaction.native_scripts = [native_script]).

@cffls
Copy link
Collaborator

cffls commented Jun 20, 2023

Thanks for the info. Could you provide the full stack trace of the error above? It seems a bug that could be fixed.

@cardano-apexpool
Copy link
Author

This is the complete error:

Traceback (most recent call last):
  File "01.multisig_raw_transaction.py", line 69, in <module>
    transaction_body = transaction.build(change_address=src_addr)
  File "/home/george/workspace/cardano/pycardano/venv/lib/python3.8/site-packages/pycardano/txbuilder.py", line 1075, in build
    self._set_redeemer_index()
  File "/home/george/workspace/cardano/pycardano/venv/lib/python3.8/site-packages/pycardano/txbuilder.py", line 755, in _set_redeemer_index
    redeemer.index = sorted_mint_policies.index(script_hash(script))
ValueError: ScriptHash(hex='b5c0307c6e64460bd1b14d21769b55f1fe271caaeb230f4d8f17c5bd') is not in list

@cffls
Copy link
Collaborator

cffls commented Jun 26, 2023

Thanks for the info. This change should be able to fix the problem. Let me know if the solution works. If it does, I will merge the other fix and close this one.

@cffls
Copy link
Collaborator

cffls commented Jul 17, 2023

Closing this PR, since #257 fixed the issue. Feel free to reopen this otherwise.

@cffls cffls closed this Jul 17, 2023
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.

4 participants