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

Make HeadId usable on-chain #919

Closed
uhbif19 opened this issue Jun 10, 2023 · 4 comments · Fixed by #1298
Closed

Make HeadId usable on-chain #919

uhbif19 opened this issue Jun 10, 2023 · 4 comments · Fixed by #1298
Labels

Comments

@uhbif19
Copy link
Contributor

uhbif19 commented Jun 10, 2023

Why

We use HeadId as part of out on-chain terms.
Now we need to convert it to CurrencySymbol, what makes is less type-safe.

https://github.com/mlabs-haskell/hydra-auction/blob/07740fb9cd832f934bab61dab23b4369e96d0c9c/src/HydraAuction/Types.hs#L78

What

Make HeadId serializable to on-chain Data.

How

I think HeadId may just use BuiltinByteString instead of ByteString.

@uhbif19 uhbif19 added the 💭 idea An idea or feature request label Jun 10, 2023
@ch1bo ch1bo self-assigned this Jun 12, 2023
@ch1bo
Copy link
Member

ch1bo commented Jun 12, 2023

So, you would like to have a HeadId type in the hydra-plutus package to refer to Hydra Head on-chain and the knowledge that this is in fact just a policy id / CurrencySymbol should be hidden? How would your code acquire such a HeadId and expect it to be used?

I see some usage of it here in your code base.

The type you are using right now is the one from Hydra.Chain which is used in the HeadLogic and on the API of the hydra-node and deliberately not cardano-specific! Making this a newtype around the plutus-tx specific BuiltinByteString is thus not desirable.

What we can do, however is to define such a type in the hydra-plutus package along with some conversion functions like headIdToCurrencySymbol if that helps you.

@ch1bo ch1bo removed their assignment Jun 12, 2023
@uhbif19
Copy link
Contributor Author

uhbif19 commented Jun 14, 2023

How would your code acquire such a HeadId and expect it to be used?

It is used as param of AuctionTerms. On chain script check that correct Head is used, and not any other head.

But it is also used on delegate servers and clients, to ensure that correct head is used.
So it requires bunch of back-and-forth type conversions that way.

Making this a newtype around the plutus-tx specific BuiltinByteString is thus not desirable.

Why? Anyway ToData/FromData are just as good, I believe. Actually we could do it ourself.
But I only see headIdToCurrencySymbol, no other side conversion. But probably it could be crafted too.

@ch1bo
Copy link
Member

ch1bo commented Jun 15, 2023

Making this a newtype around the plutus-tx specific BuiltinByteString is thus not desirable.

Why?

Because HeadId is not cardano-specific. We don't want to wrap a cardano/plutus specific type in it. Conversion will always be needed.

I'd we do the following to address this item:

  • Document the existing HeadId to make it clear it is a generic identifier for a hydra head. As this is used on the API, this will eventually also move into an api package. Let's call this Hydra.API.HeadId.

  • Introduce a new type in hydra-plutus, also named HeadId (?) and make that a type alias around CurrencySymbol (or a newtype if it does not add overhead via plutus-tx). Let's call this Hydra.Plutus.HeadId

  • Provide conversion functions to convert between the to forms, similar as we do for the ContestationPeriod and Party types. Question only is.. where? This converts from a generic head id into a cardano-specific head-id. So naturally hydra-plutus would be better suited, but that would make hydra-plutus depend on hydra-api (which does not yet exist).

@uhbif19
Copy link
Contributor Author

uhbif19 commented Jun 15, 2023

@ch1bo

Introduce a new type in hydra-plutus, also named HeadId

I proposed single type cuz it seems simple solution to me. If not single type, than I think that conversion functions are good enough, and child packages may do onchain newtype themselves, cuz this type is not used by hydra so there is no need for interoperability.

@ch1bo ch1bo added task Subtask of a bigger feature. red bin and removed 💭 idea An idea or feature request task Subtask of a bigger feature. labels Aug 8, 2023
@v0d1ch v0d1ch mentioned this issue Feb 8, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants