add_inputs, add_outputs & apply method to facilitate method chaining
#124
Conversation
|
Is this still a WIP merge request? For updating Regarding the new |
Lucid's |
Codecov Report
@@ Coverage Diff @@
## main #124 +/- ##
==========================================
- Coverage 86.36% 85.38% -0.99%
==========================================
Files 24 24
Lines 2647 2661 +14
Branches 514 518 +4
==========================================
- Hits 2286 2272 -14
- Misses 267 295 +28
Partials 94 94
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Agreed with @daehan-koreapool that creating a new function add_inputs is probably better.
I am fine with adding this apply function, since it is required for chaining calls (although I am not a big fan of it 😅).
pycardano/txbuilder.py
Outdated
| for o in utxo: | ||
| self.inputs.append(o) |
There was a problem hiding this comment.
self.inputs.extend(utxo) is simpler.
There was a problem hiding this comment.
.
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_outputstoo, so more auto-completion options in editors - More code
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I don't mind haha - maybe do an emoji poll on Discord to see what most people would prefer
There was a problem hiding this comment.
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.
pycardano/txbuilder.py
Outdated
|
|
||
| Returns: | ||
| TransactionBuilder: Current transaction builder. | ||
| """ | ||
| self.inputs.append(utxo) | ||
| if callable(callback): | ||
| callback() |
There was a problem hiding this comment.
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 selfDoing 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
)
)
)There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 selfdef 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
)
)
)There was a problem hiding this comment.
I thought of this as well and tried it, but the problem is that it gives a type error.
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😄
There was a problem hiding this comment.
Actually, that gives a type error again when you remove the optional args from add_utxos_with_ten_ada
There was a problem hiding this comment.
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 selfSo 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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Alright, makes sense. Avoids use of lambdas altogether
|
I do agree that supporting a list of UTxOs is better since multiple UTxOs will be used in most of the realistic cases. In my opinion, we could introduce new Syntactically, the users providing a single itemed UTxO with ex) https://stackoverflow.com/questions/2536307/decorators-in-the-python-standard-lib-deprecated-specifically ex) https://docs.djangoproject.com/en/dev/internals/deprecation/ |
I like this idea. However, I would like to see |
I agree with this. With regards to |
Perhaps the Deprecated package can aid with this https://pypi.org/project/Deprecated/ from deprecated import deprecated
@deprecated(version='1.2.1', reason="You should use another function")
def some_old_function(x, y):
return x + y |
Importing future and using "|" sounds fine to me. |
Yes, let's introduce this dependency. A side topic on this, currently there is no version info embedded in pycardano itself, I guess we need to introduce a version module to it, so users can easily tell the version from some call like this: import pycardano
pycardano.__version__
"0.7.0" |
Doesn't work for type aliases though, so Union will have to remain for those. |
…hods; new dependency Deprecated
|
Maybe best to add them as two seperate methods for now as suggested by @daehan-koreapool, with no deprecations yet. In the future when the number of TransactionBuilder methods increase it'll probably be wise to remove the single UTxO variants. def construct_tx1(self) -> Transaction:
builder1 = TransactionBuilder(chain_context)
(
builder1
.apply(self.add_utxos_with_ten_ada)
.add_output(
TransactionOutput(address = script_addr, amount = 5_000_000),
datum = some_datum...,
add_datum_to_witness = True
)
.add_outputs(
TransactionOutput(address = usr2_addr, amount = 5_000_000),
TransactionOutput(address = usr2_addr, amount = 5_000_000),
TransactionOutput(address = usr2_addr, amount = 8_000_000)
)
)
|
Et9797
changed the title
add_input + apply method to facilitate method chainingadd_inputs, add_outputs & apply method to facilitate method chaining
| @@ -310,6 +344,24 @@ def add_output( | |||
| self.datums[datum_hash(datum)] = datum | |||
| return self | |||
|
|
|||
| def add_outputs(self, tx_out: TransactionOutput, *tx_outs) -> TransactionBuilder: | |||
There was a problem hiding this comment.
Can we have the parameter type as a List[TransactionOutput], similar to add_inputs?
There was a problem hiding this comment.
I do agree List[TransactionOutput] is better even though it's pretty clever to allow users to provide multiple TransactionOutput through argument unpacking.
There was a problem hiding this comment.
Agree, but I wouldn't recommend not allowing varargs at all, because I think there's a strong case here for having them, as it saves typing out list brackets every time. The option to have List[TransactionOutput] + varargs as well as a single TransactionOutput + varargs would make it even more flexible syntactically imo.
In contrast, I don't think varargs syntax is justified for add_inputs, because most of the times you'll just be using filtered UTxOs retrieved from chain_context.utxos rather than writing your own UTxO objects yourself:
def construct_tx1(self) -> Transaction:
utxos: list[UTxO] = filter_func(chain_context.utxos(addr)) # more commonly
builder1 = TransactionBuilder(chain_context)
(
builder1
.add_inputs(
UTxO(
TransactionInput(tx_hash, id )
TransactionOutput(addr, amount, ....)
), # rarely
UTxO(
TransactionInput(txhash, id),
TransactionOutput(addr, amount, ....)
) # rarely
)
)
(
builder1
.add_inputs(utxos) #more commonly
)Without varargs, extra brackets required:
def construct_tx1(self) -> Transaction:
builder1 = TransactionBuilder(chain_context)
(
builder1
.add_outputs([
TransactionOutput(address=usr2_addr, amount=5_000_000),
TransactionOutput(address=usr2_addr, amount=5_000_000),
TransactionOutput(address=usr2_addr, amount=8_000_000)
])
)There was a problem hiding this comment.
I see your points. I was thinking more about the consistency in these APIs. What do you think of this:
def add_inputs(self, *utxos: UTxO]) -> TransactionBuilder
def add_outputs(self, *tx_outs: TransactionOutput) -> TransactionBuilderThis will make add_inputs and add_outputs more similar and less inconsistent, and users can pass both list and multi-args to them.
Outputs passed as a list:
my_output_list = get_output_list() # Get an output list from somewhere
builder1 = TransactionBuilder(chain_context)
(
builder1
.add_outputs(*my_output_list)
)Outputs passed as multiple positional args:
builder1 = TransactionBuilder(chain_context)
(
builder1
.add_outputs(
TransactionOutput(address=usr2_addr, amount=5_000_000),
TransactionOutput(address=usr2_addr, amount=5_000_000),
TransactionOutput(address=usr2_addr, amount=8_000_000)
)
)There was a problem hiding this comment.
Yes, I think this * version or the alternative List[...] only variant would then be the most succinct solutions. In the latter case then just stick with having to add brackets.
Only drawback with the * version is that it may seem that the * argument is optional, which it really isn't. Maybe good to have a check to see if it is empty and throw an error if it is. Not strictly required though because if it is empty it will just extend an empty tuple which doesn't throw a runtime error
There was a problem hiding this comment.
Cool, my preference is still using List[...], as it is more commonly used in Python.
There was a problem hiding this comment.
Only drawback with the * version is that it may seem that the * argument is optional, which it really isn't. Maybe good to have a check to see if it is empty and throw an error if it is. Not strictly required though because if it is empty it will just extend an empty tuple which doesn't throw a runtime error
This concern is valid in my opinion.
When things are vague, explicit is better than implicit just like zen of python.
List[...] seems better for users since IDEs will display the expected type more clearly.
|
Hi @Et9797 , any update on this PR? |
Completely forgot about this! Been consumed with other stuff. I think the need for this PR is not high, therefore perhaps sideline it for now. If you want to add add_outputs/add_inputs you can of course. The problem with chaining is the bracket requirement in Python unlike in Javascript, which kind of makes it a visual annoyance and thing to think about. A library that I use frequently called plotnine, which ports R's ggplot requires similar syntax, which can be a bit annoying if you use it a lot. |
|
Thanks for the response. I will close this draft for now. Please feel free to reopen it if needed in the future. |
Motivation
To facilitate method chaining syntax when building transactions. Without
applyit is less convenient to apply certain logic to a tx.The proposed
.applymethod takes a callback function to apply to the transaction.A minimal working example