-
Notifications
You must be signed in to change notification settings - Fork 11
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
Revamping paysXXX with more flexible payments #459
base: main
Are you sure you want to change the base?
Conversation
a2a29db
to
37b9596
Compare
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.
Is it worth having other smart constructors to remember to use (and maintain) when the user may just use the classic ones and provide mempty
/ada 0
/def
or Nothing
/TxSkelNoDatum
themselves?
…elout' into mm/txoutref-to-txskelout
…ments-smart-constructors
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.
I am not a very big fan of the new concrete API but the inner changes and the Payable
seem neat.
In my previous message, I was saying I'd rather have explicit mention of the value and datum all the time, even when not there (with explicit mempty
or Nothing
) than many different smart constructors with various names for the different combinations of "there is a value"/"no value"/"datum"/"no datum" etc. The reason is I think that there is something neat and readable in a predictable structure that is always there: address + value + datum.
Now that things can be passed in any order, and be optional, that makes the definition of an output a bit too liberal and, in the end, in my opinion only (this is very subjective) less consistent and readable in the big picture.
Besides, once again this is only personal preference and I know from experience we are opposed on that 😄, I think infix operators/functions hinder readability and writability. I'd always rather have a classic prefix function (pays/outputs/whatever) followed by a list of parameters, knowing the action before the subject. In code formatting, it often plays better, and it is easier to write. I know that you put effort into setting up the right precedence for the operator here but still, there is often some unexpected edge case that comes to bite us eventually. That's just a cosmetic remark.
I think we gained a lot in terms of ease of use and readability when we dropped the very old way to define transactions skeletons through combination of constraints involving custom operators and instead chose a very simple product type with named fields and default values for each of them. That change was more verbose but more readable. I have the impression that the current change goes in the opposite direction. In fact, maybe we need the same thing we have for skeleton adapted to outputs? Could we have an outputTemplate
where we override fields such as outputValue
, outputDatum
, outputRefScript
, etc?
This is a very big text just to express a personal opinion, don't take it for more than what it is 👍
Thanks for the feedback @florentc I understand your concern. I can at least reintroduce the old constructor |
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.
I think the general approach is better than what was previously there. Having a class makes the usage more streamlined and uniform at the callsite, instead of having to use a dozen of specialized helpers.
However, maybe because I'm not a die hard Haskeller, I can only agree with @florentc on the operator usage. I generally find that Haskell is already heavy on operators, which means that you need to keep much more in your head (combine
or mappend
is more self-explanatory than <>
, if you come from another language).
Adding idiosyncratic operators is worse: now you need to know operators specific to one library. I'm not sure it's worth it, as the gain is small here IMHO: infix functions in Haskell makes it already ok to use functions as operators, and adding parentheses to the right of receives
isn't a big deal. In fact, I would even say that the current precedence of &>
quite confusing, because for all built-in operators in any language out there, function application has usually always higher precedence than infix operators: f x && y
almost always means (f x) && y
, and f x + g y
always mean (f x) + (g y)
and not f (x + g y)
. So my brain has a natural tendency of parsing foo `receives` bar &> baz
as (foo `receives` bar) &> baz
, which I need to consciously fight against.
Though, it is a matter of taste, and I know not everyone agree 🙂
@@ -15,14 +15,14 @@ alice, bob :: Wallet | |||
-- type Int and value 10 for each datum kind | |||
initialDistributionWithDatum :: InitialDistribution | |||
initialDistributionWithDatum = | |||
InitialDistribution $ [withDatum, withInlineDatum] <*> [paysPK alice (Script.ada 2)] <*> [10 :: Integer] | |||
InitialDistribution $ [receives alice . (Script.ada 2 &>)] <*> ([TxSkelOutDatum, TxSkelOutInlineDatum] <*> [10 :: Integer]) |
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.
Maybe it's my Haskell noob eyes, but I find this expression a bit hard to parse/decipher.
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.
Yes this is hard to parse I agree.
However:
- it was already hard to parse in the previous version, because it uses the somewhat uncommon
<*>
operator. - it is in the test suite so I feel it is less important that everything is easily parseable there but this is debatable.
The idea was to avoid duplicates in the list definition, but really I could make it much more simple and readable.
About this, a possibility would be to not make |
Thank you very much for your feedback @florentc and @yannham. While reading your messages, I realized that I have not motivated this contribution enough, a wrong that I intend to right in this message. To me this change is absolutely necessary for a lot of reasons:
Overall, I found this API very inconvenient to use. Not terrible by any means, as we used it a lot, but inconvenient in the long run, and what is inconvenient can very likely be improved. However, regarding the content of my contribution itself. I completely agree that it can be improved. In particular, my operator is a bit random and the priorities are convenient but impractical and lead to code chunks unusual to parse. This is however just the tip of the iceberg. The idea here is to have a composable notion of payment that leads to a simpler (and smaller) API, containing basically only two things: having some party receiving some payable structure, and being able to compose payable elements. The issue is that you cannot really have an instance of Monoid on things that can implement an interface, which is why I had to add a custom operator. As the monoid operator is widely used in the community, this would have been very easily accepted by users. If we want to adopt the "txSkel" way of doing things with a default payment that users can fill out, this is possible but it has drawbacks:
Another solution would be to just remove the priorities of the operators I defined (forcing users to parenthesis their payments, which is sensible) and maybe have a function instead of an operator to combine payable elements that could be used infix if needed, in which case we have to find a sensible name for it. I argue that in any case users have to remember something: either the function name or the operator name. This is unavoidable but, admittedly, a function name is usually easier to remember because it is made of actual meaningful words. To summarize, I feel like this contribution is essential in our journey to improving cooked validator API, but I'm very open to suggestions as to how to overcome the limitations of my current attempt, which is not without challenges. |
I agree - a We can also reconsider later if this first simple approach has too much friction.
On this point, it seems that you are mostly considering writing code 🙂 my personal reservations rather revolve around reading (indeed, when writing, you need to look for the proper function/operator to use anyway). IMHO a casual reader will have a harder time making sense of an arbitrary operator with an arbitrary precedence rather than a named function (if we can pick a good name). |
This PR introduces two payment smart constructors to allow paying a script with no specified value. This idea is to rely on the optiontxOptEnsureMinAda
which should not force users to specify a value when activated.Following @florentc comments and some more thoughts, this PR actually provides a new convenient universal way of building payments by having a
Payable
structure that can be populated by various elements.The old helpers are still there, but are now built from it.Typically, following this change all payments will look like:
or:
As a side consequence, this in fact solves the initial intent of this PR, as nothing forces users to specify a value if they wish it to be computed automatically from the min ada restriction. Such a statement will be totally valid:
Some remarks/comments/questions:
It is currently possible to pay nothing at all (using
mempty
) which should not have many practical use cases.This PR also renders obvious the prevalent nature of the recipient in the payment. While our payment have the recipient as one of many field, it is actually the only one that is mandatory. There is always a recipient to a payment. This changes makes is more obvious.
I randomly chose
(&>)
as an operator to compose payable elements, but I'm open to better suggestions.I don't like that the users have to tediously use the long constructors for
TxSkelOutDatum
. Remove theTxSkelOut
part would help, but maybe there's an even better way to circumvent this, but I have not yet found any.The
Payable
structure is composable and relies of the alternative instance for Maybe for all its fields excepts values. Values are instead composed using their semigroup instance. This means that if you pay, let's say, a reference script twice, only the second instance will be kept, while if you pay values twice, they will be added, which should be very convenient.To avoid have to parenthesis everything, I set proper priorities for both introduced operators (
receives
and&>
) so that they can be used conveniently together. This means that the latter has priority over the former.To emphasize the usefulness of
receives
and lighten the library, I removed all smart constructors related to payments. This has the only drawback of disconnecting typed validators to the type of their datum when paying to them. This is debatable whether that's a concern or not.