Skip to content

add_inputs, add_outputs & apply method to facilitate method chaining #124

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.

Sign up for GitHub

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 2 commits into from
Closed

add_inputs, add_outputs & apply method to facilitate method chaining #124

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d091a19
introduced apply & add_input supports list of utxos
Nov 5, 2022
46ca732
callback takes *args and **kwargs; added add_inputs & add_outputs met…
Nov 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 25 additions & 5 deletions pycardano/txbuilder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

from copy import deepcopy
from dataclasses import dataclass, field, fields
from typing import Dict, List, Optional, Set, Tuple, Union
from typing import Dict, List, Optional, Set, Tuple, Union, Callable

from pycardano.address import Address, AddressType
from pycardano.backend.base import ChainContext
Expand Down Expand Up @@ -143,16 +143,36 @@ class TransactionBuilder:

_should_estimate_execution_units: bool = field(init=False, default=None)

def add_input(self, utxo: UTxO) -> TransactionBuilder:
"""Add a specific UTxO to transaction's inputs.
def apply(self, callback: Callable[[], None]) -> TransactionBuilder:
"""Apply the provided callback function to the transaction.

Args:
utxo (UTxO): UTxO to be added.
callback (Callable[[], None]): A callback function. The callback
should not return a value, as nothing will be done with it.

Returns:
TransactionBuilder: Current transaction builder.
"""
self.inputs.append(utxo)
if callable(callback):
callback()
Copy link
Collaborator

Choose a reason for hiding this comment

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

A simpler approach is make the type of this callable to take the builder as the input, like this:

def apply(self, callback: Callable[[TransactionBuilder], None]) -> TransactionBuilder:
    if callable(callback):
        callback(self)
    else:
        raise ValueError("Not a callable.")
    return self

Doing so, we don't need to create a lambda function in caller's code:

def add_utxos_with_ten_ada(builder):
  for utxo in chain_context.utxos(str(usr1_addr)):
    if utxo.output.amount == 10_000_000:
      builder.add_input(utxo)

class CustomTxBuilder:

  def construct_tx1(self) -> Transaction:
     builder1 = TransactionBuilder(chain_context)
    (
    builder1
      # .add_input(chain_context.utxos(str(usr1_addr))) # list of UTxOs
      .apply(add_utxos_with_ten_ada)
      .add_output(
        TransactionOutput(
          address = usr2_addr,
          amount = 20_000_000
        )
      ) 
      .add_output(
        TransactionOutput(
          address = usr2_addr,
          amount = 20_000_000
        )
      ) 
    )

Copy link
Author

Choose a reason for hiding this comment

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

Good point. Only drawback is that you're now required to have a builder parameter in the callback every time. And when the callback is going to take more arguments than just the builder, you'll end up using a lambda again.

def add_utxos_with_ten_ada(builder, p1, p2):
    ....

Now the lambda will need to take a builder arg too, like the following:
apply(lambda builder: add_utxos_with_ten_ada(builder, arg1, arg2)
As opposed to:
apply(lambda: add_utxos_with_ten_ada(builder1, arg1, arg2)

I think the number of times you'll have to apply additional params is a handful of cases though, so in that case it's easier with your suggestion because it avoids the lambda altogether. On the other hand lambdas can be a bit confusing and having to provide no input is also convenient

Copy link
Collaborator

Choose a reason for hiding this comment

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

This problem could be solved by using (*args, **kwargs) like this:

def apply(self, callback: Callable[[TransactionBuilder, ], None], *args, **kwargs) -> TransactionBuilder:
    if callable(callback):
        callback(self, *args, **kwargs)
    else:
        raise ValueError("Not a callable.")
    return self
def add_utxos_with_ten_ada_with_params(builder, to_print, my_keyword=None):
  for utxo in chain_context.utxos(str(usr1_addr)):
    if utxo.output.amount == 10_000_000:
      builder.add_input(utxo)
  print(to_print)
  print(my_keyword)

class CustomTxBuilder:

  def construct_tx1(self) -> Transaction:
     builder1 = TransactionBuilder(chain_context)
    (
    builder1
      # .add_input(chain_context.utxos(str(usr1_addr))) # list of UTxOs
      .apply(add_utxos_with_ten_ada, "my_positional_arg", my_keyword="my_keyword_arg")
      .add_output(
        TransactionOutput(
          address = usr2_addr,
          amount = 20_000_000
        )
      ) 
      .add_output(
        TransactionOutput(
          address = usr2_addr,
          amount = 20_000_000
        )
      ) 
    )

Copy link
Author

Choose a reason for hiding this comment

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

I thought of this as well and tried it, but the problem is that it gives a type error.

image

We need to add *args and **kwargs to the type signature of callback. I tried looking up if there's a canonical way of doing this, but there doesn't seem to be until 3.10. Assuming Any type does work:
def apply(self, callback: Callable[[TransactionBuilder, Any, Any], None], *args, **kwargs) -> TransactionBuilder:
Not the prettiest solution😄

Copy link
Author

@Et9797 Et9797 Nov 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, that gives a type error again when you remove the optional args from add_utxos_with_ten_ada

Copy link
Author

@Et9797 Et9797 Nov 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Only solution to make it type correct so far seems to be callback: Callable[..., None].
https://stackoverflow.com/a/68472403/12541069
However, this also means TransactionBuilder is an optional arg, which it shouldn't be. Therefore I propose for now to just restrict it to TransactionBuilder as only arg, and allow no other added optional arguments in the callback:

def apply(self, callback: Callable[[TransactionBuilder], None]) -> TransactionBuilder:
    if callable(callback):
        callback(self)
    else:
        raise ValueError("Not a callable.")
    return self

So basically your first suggestion I think is best. A user can still add parameters if they really need to. They'll have to add a lambda in that case. But in most cases it'll just be the direct callback function without a lambda.
apply(lambda builder: add_utxos_with_ten_ada(builder, arg1, arg2)

Copy link
Collaborator

@cffls cffls Nov 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for experimenting these options. I don't think it is worth enforcing strict typing in this use case, because apply is supposed to be very generic, and we are kind of creating a problem for ourselves here. I prefer using callback(self, *args, **kwargs) and put a @typing.no_type_check decorator for apply to bypass mypy type checking. This will make everything concise again.

Copy link
Author

@Et9797 Et9797 Nov 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Alright, makes sense. Avoids use of lambdas altogether

else:
raise ValueError("Not a callable.")
return self

def add_input(self, utxo: Union[UTxO, List[UTxO]]) -> TransactionBuilder:
"""Add a specific UTxO or a list of UTxOs to the transaction's inputs.

Args:
utxo (Union[UTxO, List[UTxO]]): UTxO or list of UTxOs to be added to the transaction.

Returns:
TransactionBuilder: Current transaction builder.
"""
if isinstance(utxo, list):
for o in utxo:
self.inputs.append(o)
Copy link
Collaborator

Choose a reason for hiding this comment

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

self.inputs.extend(utxo) is simpler.

Copy link
Author

@Et9797 Et9797 Nov 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

.

If add_inputs is added, then it probably makes sense as well to add an .add_outputs method. So now you're introducing two methods.. which will make the auto-completion list grow in editors😄
One argument for having two separate functions though would be that for new devs the Union syntax could be a bit confusing to read. Hence why I like the new alternative | union type hint introduced in 3.10: UTxO | List[UTxO]
So what I can think of for now, the pros and cons for separate methods:

  • Unambiguous
  • Probably clearer for new devs

Cons:

  • Will need an .add_outputs too, so more auto-completion options in editors
  • More code

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am fine with either way, will let you decide :D For union type, we will probably still need to use Union, because the library needs to support not only 3.10+, but anything above 3.7.

Copy link
Author

Choose a reason for hiding this comment

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

I don't mind haha - maybe do an emoji poll on Discord to see what most people would prefer

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we need to support Python3.7 and up, we should stick to Union type hint for the time being throughout the entire code base.

Mypy is set to target Python3.7 for type static analysis as well.

else:
self.inputs.append(utxo)
return self

def _consolidate_redeemer(self, redeemer):
Expand Down