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.

Sign up for GitHub

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

Jump to bottom

Document changes to BaseAddr #619

Merged
merged 4 commits into from
Dec 4, 2024
Merged

Document changes to BaseAddr #619

merged 4 commits into from
Dec 4, 2024

Conversation

williamdemeo
Copy link
Contributor

@williamdemeo williamdemeo commented Dec 2, 2024 •
edited
Loading

Description

This closes issue #617.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • Any semantic changes to the specifications are documented in CHANGELOG.md
  • Code is formatted according to CONTRIBUTING.md
  • Self-reviewed the diff

@williamdemeo williamdemeo linked an issue Dec 2, 2024 that may be closed by this pull request
Document changes to BaseAddr #617
Closed
@williamdemeo williamdemeo marked this pull request as ready for review December 2, 2024 20:47
@williamdemeo williamdemeo requested a review from lehins December 2, 2024 20:49
lehins
lehins approved these changes Dec 3, 2024
View reviewed changes
Copy link

@lehins lehins left a comment

Choose a reason for hiding this comment

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

Perfect! Thank you!

@williamdemeo williamdemeo merged commit c59a4e3 into master Dec 4, 2024
8 checks passed
@williamdemeo williamdemeo deleted the 617-document-changes-to-baseaddr branch December 4, 2024 17:08
github-actions bot added a commit that referenced this pull request Dec 4, 2024
@github-actions
Generate code for GH-619
6dbe13f
@WhatisRT
Copy link
Collaborator

WhatisRT commented Dec 5, 2024

Sorry, this isn't good enough. A base address has a staking credential, see e.g. https://docs.cardano.org/about-cardano/learn/cardano-addresses/:

A base address directly specifies the staking key that should control the stake for that address.

So the new BaseAddr type doesn't correspond to a base address, it corresponds to a base or enterprise address. I'd have suggested to rename it to Addr, but that name is already taken for the purpose of 'any address that can be used in spending'. So maybe BaseOrEnterpriseAddr? BEAddr? I don't know, but BaseAddr is definitely not a good name for this type.

@WhatisRT WhatisRT mentioned this pull request Dec 5, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lehins lehins lehins approved these changes

@Soupstraw Soupstraw Awaiting requested review from Soupstraw

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Document changes to BaseAddr
3 participants
@williamdemeo @WhatisRT @lehins