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

Add support for StakeAddress and BaseAddress output #46

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kevink1103
Copy link

@kevink1103 kevink1103 commented Nov 23, 2022

This PR includes:

  • Deprecate Wallet.Addresses() and replace it with Wallet.EnterpriseAddresses()
  • Deprecate Wallet.Keys() and replace it with Wallet.PrvKeys()
  • New Wallet.BaseAddresses()
  • StakeAddress support for Address and Wallet
  • Add PoolCost in poolRegistration struct

closes #45
closes #20

@kevink1103
Copy link
Author

kevink1103 commented Nov 23, 2022

Looks like I just made a duplicate feature of #41 ... but my PR includes tests for the feature.
Choice is up to you, @echovl .

@kevink1103 kevink1103 changed the title Add support for StakeAddress output Add support for StakeAddress and BaseAddress output Nov 23, 2022
@safanaj
Copy link

safanaj commented Nov 24, 2022

Regarding the StakeAddress management this and #41 are pretty equivalent, I can split #41 to just add easy-to-use bech32 end/dec, and get this merged.
Pls @echovl give us a feedback

@kevink1103
Copy link
Author

kevink1103 commented Nov 24, 2022

@safanaj it was too late when I finished implementing StakeAddress management on my own and later found out you've done similar work in #41 PR...
I would appreciate it if you could split your PR to only include bech32 utilities and have it merged first because I think my PR takes care of more address-wise changes (baseAddress, some function changes in Wallet, tests...). Then, I'll update my code with some of your new works.

@kevink1103
Copy link
Author

kevink1103 commented Nov 24, 2022

@echovl Could you allow few more people to review PRs and have write access to the repo so that they can merge PRs as well? Hope this repo to be a bit more responsive and prosper :)

@safanaj
Copy link

safanaj commented Nov 28, 2022

@safanaj it was too late when I finished implementing StakeAddress management on my own and later found out you've done similar work in #41 PR... I would appreciate it if you could split your PR to only include bech32 utilities and have it merged first because I think my PR takes care of more address-wise changes (baseAddress, some function changes in Wallet, tests...). Then, I'll update my code with some of your new works.

Agree, yes I will. Only I saw that on my PR in address.go there are 2 typo fixes in the NewAddressFromBytes constructor, regarding the error message for Ptr address type if those make sense to you too add them.

@echovl if you are still short with time probably as suggested by @kevink1103 you could allow us to review and merge.

@kevink1103
Copy link
Author

kevink1103 commented Nov 28, 2022

@safanaj Thank you! I've just taken care of the typos you've mentioned. Let's wait for @echovl 's review!

@kevink1103
Copy link
Author

kevink1103 commented Dec 5, 2022

As this PR will adopt changes from #41 , I will make this PR as draft for now so that #41 gets merged first. cc. @safanaj

@kevink1103 kevink1103 marked this pull request as draft December 5, 2022 22:34
@echovl
Copy link
Owner

echovl commented Dec 15, 2022

Hi guys, sorry for the late reply, this looks good to me. Do we really need the bech32 PR merged first?

@kevink1103
Copy link
Author

kevink1103 commented Dec 15, 2022

@echovl Hi, welcome back!
and whichever is fine for me. As soon as #41 PR gets merged, I can take advantage of prefixes defined there. So we can still merge this one first. I'll put this PR ready for review.

@kevink1103 kevink1103 marked this pull request as ready for review December 15, 2022 23:51
@kevink1103
Copy link
Author

Just to follow up on this PR.
Since this PR has not been paid enough attention and reviewed, I have forked this repo and added more features on top of this PR.
https://github.com/ffdd/cardano-go

For those who are interested in this PR, use the forked one!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for StakeAddress output Cannot generate same address use same mnemonic with cardano java lib
3 participants