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

CIP-0121? | Integer-ByteString conversions #624

Merged
merged 23 commits into from
Jun 11, 2024

Conversation

kozross
Copy link
Contributor

@kozross kozross commented Nov 21, 2023

CIP for Plutus primops for converting BuiltinInteger to BuiltinByteString, and back.

Human-readable version.

@Ryun1 Ryun1 added the Category: Plutus Proposals belonging to the 'Plutus' category. label Nov 21, 2023
@Ryun1 Ryun1 changed the title Integer <-> ByteString conversions CIP-???? | New Plutus built-ins builtinIntegerToByteString and builtinByteStringToInteger Nov 21, 2023
@rphair rphair changed the title CIP-???? | New Plutus built-ins builtinIntegerToByteString and builtinByteStringToInteger CIP-???? | Integer-ByteString conversions Nov 27, 2023
Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

Thanks @kozross for a well documented submission. First I'm retitling this by convention the same as the current document title (Integer-ByteString conversions) especially since the more succinct title will work better in our CIP list once merged.

The main item I believe needs to be fixed (or checked at least) are the CIP-0035 requirements for builtin changes. Though different language is used, I think the general requirements are satisfied: https://github.com/cardano-foundation/CIPs/blob/master/CIP-0035/README.md#processes

... but not all the particular requirements: https://github.com/cardano-foundation/CIPs/blob/master/CIP-0035/README.md#additions-to-the-plutus-core-builtins

@michaelpj any time you can look over this submission I would really appreciate it. If I'm wrong about this not meeting the particular specifications for builtins by CIP-0035 I would happily stand corrected. To me this looks ready for acceptance as soon as all the formalities are met.

CIP-0087/CIP-0087.md Outdated Show resolved Hide resolved
CIP-XXXX/README.md Outdated Show resolved Hide resolved
CIP-XXXX/README.md Outdated Show resolved Hide resolved
CIP-XXXX/README.md Outdated Show resolved Hide resolved
CIP-XXXX/README.md Outdated Show resolved Hide resolved
CIP-XXXX/README.md Outdated Show resolved Hide resolved
@zliu41
Copy link
Contributor

zliu41 commented May 2, 2024

@rphair Can we assign this a CIP number and merge it? Not only has it already been approved, but it has already been implemented and released in Plutus V3.

@rphair rphair self-assigned this May 3, 2024
@rphair
Copy link
Collaborator

rphair commented May 3, 2024

@zliu41 my apologies for being under the impression that the currently debated #794 was relative to CIP-0058 alone; in fact this one had fallen through the cracks in the belief it was still waiting for input from Plutus reviewers.

This PR branch is on an MLabs repository so I'll propose some changes there which @kozross can approve, to add a CIP number 121 and update the CIP repository README, so we don't have to wait for the next CIP meeting. Once that's committed then I'll approve this PR & expedite confirmations from other editors: shouldn't take too long after that.

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

@kozross I'll also submit a PR against this branch to modify the README for the CIP repository that officially assigns the CIP number (otherwise we wait for the 2-week meeting intervals).

CIP-XXXX/README.md Outdated Show resolved Hide resolved
CIP-XXXX/README.md Outdated Show resolved Hide resolved
CIP-XXXX/README.md Outdated Show resolved Hide resolved
@kozross this is likely to cause a merge conflict with the CIP repo's `master` branch; if so, just put your new CIP `121` after the merged CIP `120` and keep the more current modification date.
@rphair rphair changed the title CIP-???? | Integer-ByteString conversions CIP-0121? | Integer-ByteString conversions May 3, 2024
@kozross kozross requested review from zliu41 and rphair May 5, 2024 18:44
Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

@kozross look ready to merge from an editorial perspective, except for one artefact in the repo README probably from rebasing or fixing a merge conflict; will approve this when that's fixed...

README.md Outdated Show resolved Hide resolved
Co-authored-by: Robert Phair <rphair@cosd.com>
@rphair
Copy link
Collaborator

rphair commented May 20, 2024

@Ryun1 @Crypto2099 I'm marking this as Last Check since it seems that the same review process as #806 has been followed as noted in #806 (review) (cc @kozross) and so putting it on the agenda as such (https://hackmd.io/@cip-editors/89).

Likewise @michaelpj @zliu41 please let us know if any technical reservations. It seems to me everything above has been addressed & I would be in favour of merging these 2 related proposals before the meeting if the other editors agree.

@rphair rphair added the State: Last Check Review favourable with disputes resolved; staged for merging. label May 20, 2024
CIP-0121/README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

@kozross re: #624 (review) thanks for eaa2263 but there's still a merge conflict due the modification time on that section also being changed:

README.md Outdated Show resolved Hide resolved
Co-authored-by: Robert Phair <rphair@cosd.com>
@rphair rphair requested review from Crypto2099 and zliu41 and removed request for zliu41 May 29, 2024 20:07
@rphair
Copy link
Collaborator

rphair commented May 29, 2024

@zliu41 apologies for extra tags above... I fat fingered some GitHub feature while my Internet connection was bad. I understand you think this is ready to merge and FYI the editorial consensus from CIP meeting 2 days ago was that this is ready to go pending the administrative requirements that have recently been addressed.

@rphair rphair merged commit c304c5f into cardano-foundation:master Jun 11, 2024
@rphair rphair removed the State: Last Check Review favourable with disputes resolved; staged for merging. label Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Plutus Proposals belonging to the 'Plutus' category.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants