Skip to content
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

Kwxm/costing/restore old cost model param names #5932

Merged
merged 3 commits into from
Apr 29, 2024

Conversation

kwxm
Copy link
Contributor

@kwxm kwxm commented Apr 29, 2024

The changes in #5857 caused some of the cost model parameter names (for equalsByteString and equalsString) to change, which introduced a conflict with some of the names in the Alonzo genesis file. The parameter names changed because one of the costing function types had a linear function hard coded into it and I replaced it with something where you could have a more general type. I've now added the more specific type back: it's redundant but it has the right names. See this Note for more information.

A few other points:

  • The ledger code is already adjusting some of the cost model parameter names here and that code could easily be extended to deal with this problem as well.
  • What I've done here is a stopgap to restore the old behaviour. The parameter names are automatically derived from constructor names using Aeson's FromJSON/ToJsonmethods. A better way to solve our problem might be to write custom FromJSON/ToJSON instances that convert the restricted linear_on_diagonal tag to the more general ConstantOffDiagonal constructor, so that we'd have a sort of syntactic sugar that could convert the JSON encoding of our old types into the internal version of our new types. That might take some time though, so I've gone for a quicker fix here.
  • We probably do want to use the new tags and types exclusively in Conway. I think that should be OK since my understanding is that there will be a new genesis file for the post-Conway era and we can use the new parameter names in that. This will require some changes in plutus-ledger-api and I think it'll also depend on the work that's currently in progress on allowing configurable cost models (see here for example, although there's more work to be done).

@kwxm kwxm added Ledger Costing Anything relating to costs, fees, gas, etc. No Changelog Required Add this to skip the Changelog Check labels Apr 29, 2024
@zliu41 zliu41 merged commit 4a29380 into master Apr 29, 2024
8 checks passed
@zliu41 zliu41 deleted the kwxm/costing/restore-old-cost-model-param-names branch April 29, 2024 19:16
@kwxm kwxm mentioned this pull request Apr 30, 2024
Copy link
Contributor

@effectfully effectfully left a comment

Choose a reason for hiding this comment

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

Sorry for the late review. LGTM.

It sucks that we can't change the names easily. Also relying on aeson is generally horrible, I do believe we should bite the bullet and have everything written out explicitly, even though it's a lot of boilerplate.

Could you please create a GitHub issue with the appropriate details from your original comment to make sure we don't forget about this problem?

@kwxm
Copy link
Contributor Author

kwxm commented May 1, 2024

Could you please create a GitHub issue with the appropriate details from your original comment to make sure we don't forget about this problem?

Yes, I was planning to do that (so thanks for the reminder). I think we should also keep a careful record of the history, so we can see how to relate old parameter names to new ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Costing Anything relating to costs, fees, gas, etc. Ledger No Changelog Required Add this to skip the Changelog Check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants