Questions

[fallen-icarus]

I have a few questions that I haven't had the time to fully answer myself. Considering you took a different path with the aiken code, perhaps you are better positioned to answer them. I wasn't sure if I should ask them here or in the discussion but since you made this repo private, I decided to ask the questions related to your code here.

1. Do you think it is necessary to have a dedicated Ratio type for the datums? Since the beacons will fail to mint unless the datums are valid, which includes the ratio elements, I did not see a need to create a dedicated type for the datums. Do you not think the presence of the beacons is enough to ensure validity of the ratios?
2. Do you get complicated error messages when you expect True? I like how concise it is but am concerned about the error messages being unfriendly.
3. Did you come across any issues not having ratio arithmetic automatically reduced after each operation. Specifically, I am concerned that reduce(x + y / z) != reduce(x + reduce(y / z)). This is relevant for front-ends since whatever language they use may automatically reduce after each operation which would mean a custom Rational type would be required to calculate the same ratio as the script. This may not be too much of an issue for cardano-loans since the arithmetic is pretty basic but it would be more of an issue for cardano-swaps when the weighted average asking price is calculated.

[ken-underscore]

I appreciate you taking a look and your questions! I'm still a bit of a novice so apologies if I misunderstand any of the questions.

1. No, I don't see any reason Ratio would be necessary. I think my main motivation was just ease of implementation, when I originally wrote everything out I hadn't considered that Rational was opaque and wouldn't be able to be used in the datum. When it came time to fix this I did consider just using a pair or a list to hold the numerator/denominator instead, I didn't see a strong reason to go either way at the time except maybe a Ratio type would be a bit more intuitive to someone looking at the data model from a transaction building perspective probably at the cost of some performance overhead.
2. Hm, I actually originally wrote this without using any expect True, but while I was testing via lucid I noticed that it didn't matter how the contract failed (error @, False, or expect failure) I would get the same message regardless "The provided Plutus code called 'error'." I'm guessing this isn't the case when using the CLI? Since error messages weren't helping me either way I did appreciate how expect True helped to keep context together. If error messages can actually be bubbled up to transaction builders and helped them troubleshoot then I can see why that would be important, I wonder if expect could be changed to support custom error messages on failure.
3. I did not, I was concerned about this since I wrote a Rational library originally that followed the haskell implementation and reduced after every operation. After switching to the new Rational library in stdlib I tried to think of where it could be an issue and it seemed like the only place would be the balance field on the Active datum since iterative calculations occur there, so that's currently the only place I am using reduce.
   I can't think of a scenario where reduce(x + y / z) != reduce(x + reduce(y / z)) would be true, uniqueness is guaranteed for each distinct rational number represented by an irreducible fraction. I do wonder if there is a point where more complicated arithmetic wouldn't benefit from a different strategy for reduce, but like you say there is nothing too bad in cardano-loans.

[fallen-icarus]

You can still use Rational in the datum even though the type is opaque. In the aiken branch for cardano-loans this is my version of the Rational type. Despite it being opaque, I am still able to use it in the datums here. This approach means it is 100% possible to create invalid Rationals since it bypasses the normal way they are constructed. I just realized I assumed that doing comparisons on invalid ratios would fail but this is probably not true. This should be looked into more. Perhaps the beacon policy should do some extra checks on the rationals from datums before giving the "mint of approval". This should be a cheap check.

For the error messages, did you build the aiken scripts with aiken build --keep-traces? All error messages are stripped away by default unless the --keep-traces flag is used.

You may be right that using reduce sparingly is fine. I'd like to try testing and verifying this though. For subtraction and addition it is probably fine, but for multiplication and division it isn't obvious to me that there won't be drift.

[ken-underscore]

That's interesting, I just tried out using Rational and Lucid doesn't seem to be able to generate the correct types with their blueprint function, I'll have to play with this some more, bypassing the invariant definitely sounds a potentially dangerous if not handled.

Wow, it was that easy! --keep-traces was the issue, wish I had known that while I was building the transactions, also wish I had started out using Lucid's emulator, the feedback loop is so much tighter. The expect True errors aren't too bad if you pair them with ? on the booleans, that does get a bit messy though because it often requires a set of parenthesis. If it was a standard dapp where the same team builds the contracts and the interface I think expect True might be fine, but I think your self documenting error messages will likely be invaluable to people building interfaces to these contracts.

[fallen-icarus]

I did some testing and thought I'd share my findings:

For making sure Rational datums contain valid Rationals, I believe it is enough to just check that the denominator is > 0. The two invariants for Rational are the denominator is != 0 and the negative sign must always be in the numerator. Checking that the denominator is > 0 ensures both. The beacon minting policies can do this so that the spending scripts can use the presence of the beacons to ensure the Rationals are valid.

The sparing use of reduce has not caused any issues so I will likely use it going forward. Calculating the weighted average price in cardano-swaps is the most complicated calculation I've used and yet I have not had any issues using reduce once at the end of the calculation.

I think I found a compromise for using expect True. First, I define this function:

pub fn error_if_false(msg: String, predicate: Bool) {
  if !predicate {
    error msg
  } else {
    predicate
  }
}

Then I can use the function like this:

        // The beacon_id must be the policy id of the beacon policy.
        expect True = error_if_false( @"Invalid SwapDatum beacon_id", beacon_id == beacon_id_ )

        // The beacon name must be sha2_256( ask_asset policy id ++ ask_asset asset name )
        expect True = error_if_false( @"Invalid SwapDatum beacon_name", beacon_name == beacon_name_ )

        // The offer_id must be the offer_id that this beacon policy is for.
        expect True = error_if_false( @"Invalid SwapDatum offer_id", offer_id == offer_id_ )

        // The offer_name must be the offer_name that this beacon policy is for.
        expect True = error_if_false( @"Invalid SwapDatum offer_name", offer_name == offer_name_ )

        // The ask_id must be the ask_id for this beacon.
        expect True = error_if_false( @"Invalid SwapDatum ask_id", ask_id == ask_id_ )

        // The ask_name must be the ask_name for this beacon.
        expect True = error_if_false( @"Invalid SwapDatum ask_name", ask_name == ask_name_ )

        // The price must be the required price.
        expect True = error_if_false ( @"SwapDatum price not weighted avg", price == req_price )

Since error_if_false will never actually return False, the ugly error message with expect will never be given. The error message from error_if_false will always be given first. I think this is also more legible, from a code audit perspective, than what I did in my aiken version of cardano-loans with a bunch of nested if-elses.