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

refactor(models)!: change SubaddressIndex to use subaddress::Index #62

Merged
merged 5 commits into from
Jul 19, 2022
Merged

refactor(models)!: change SubaddressIndex to use subaddress::Index #62

merged 5 commits into from
Jul 19, 2022

Conversation

LeoNero
Copy link
Member

@LeoNero LeoNero commented Jul 11, 2022

Closes #41

monero-rs/monero-rs#104 needs to be merged first

@h4sh3d
Copy link
Member

h4sh3d commented Jul 12, 2022

monero-rs version 0.17.1 has been release, this should now work!

@LeoNero
Copy link
Member Author

LeoNero commented Jul 12, 2022

@h4sh3d thank you! I'll update this PR

@LeoNero LeoNero force-pushed the change/subaddressindex branch from 70fcd8e to 2b7d3b4 Compare July 12, 2022 17:33
@LeoNero
Copy link
Member Author

LeoNero commented Jul 12, 2022

done

TheCharlatan
TheCharlatan requested changes Jul 13, 2022
View reviewed changes
Copy link
Member

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

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

I'd prefer if you use the imported Index wherever possible and also replace the currently defined tuples of indices with it. We are already using monero-rs as a user required library, so I'd prefer if we we were consistent. There is also no need to rename the Index to SubAddressIndex. This will be a breaking change to the lib.

@h4sh3d h4sh3d mentioned this pull request Jul 13, 2022
Closed
@LeoNero
Copy link
Member Author

LeoNero commented Jul 13, 2022

perfect, I'll do it :)

@LeoNero LeoNero force-pushed the change/subaddressindex branch from 2b7d3b4 to e99b312 Compare July 13, 2022 17:43
@LeoNero
Copy link
Member Author

LeoNero commented Jul 13, 2022

@TheCharlatan Done. I also used subaddress::Index instead of Index to make it easier to see it is not defined in this crate. I also changed any account_index and address_indices to use u32 and Vec<u32> respectively, in order to make it use the same types Index uses.

@LeoNero LeoNero force-pushed the change/subaddressindex branch from e99b312 to ee6d6fc Compare July 13, 2022 17:44
@LeoNero
Copy link
Member Author

LeoNero commented Jul 13, 2022 •
edited
Loading

btw I would only merge this PR after I add tests for all functions on lib.rs in another PR

TheCharlatan
TheCharlatan reviewed Jul 13, 2022
View reviewed changes
Copy link
Member

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

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

Nicely done, and yes per your suggestion let's merge once the other PR is done. I like to put the onus of maintaining pull request merge order entirely on the author. Can you put this PR back in draft state as long as you are still working on its dependencies?

@LeoNero LeoNero marked this pull request as draft July 13, 2022 18:19
@LeoNero LeoNero force-pushed the change/subaddressindex branch from ee6d6fc to 0aef439 Compare July 18, 2022 19:14
@LeoNero LeoNero marked this pull request as ready for review July 18, 2022 19:14
@LeoNero LeoNero requested a review from TheCharlatan July 18, 2022 19:14
TheCharlatan
TheCharlatan approved these changes Jul 19, 2022
View reviewed changes
Copy link
Member

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

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

ACK

@TheCharlatan TheCharlatan merged commit 16d123e into monero-rs:master Jul 19, 2022
refring pushed a commit to refring/monero-rpc-rs that referenced this pull request Oct 16, 2022
@LeoNero
refactor(models)!: change SubaddressIndex to use subaddress::Index (m…
9ab045d 
…onero-rs#62)

* refactor(models)!: change SubaddressIndex to use subaddress::Index

* refactor(get_address_index)!: return type is now Result<subaddress::Index>

* refactor(models,lib)!: change any address index and account index to use u32

* refactor(label_address)!: receive subaddress:Index as an argument

* docs(CHANGELOG): update CHANGELOG file to reflect recent changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheCharlatan TheCharlatan TheCharlatan approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace SubaddressIndex with monero Index
3 participants
@LeoNero @h4sh3d @TheCharlatan