search

revault / revaultd

The "wallet" daemon for participants under the Revault architecture
BSD 3-Clause "New" or "Revised" License
48 stars 21 forks source link

Added participant_type to get_info. #379

Closed Zshan0 closed 2 years ago

Zshan0 commented 2 years ago

Hey, I added participant_type to get_info as requested in #45.

Let me know if there are any changes to be made.

Zshan0 commented 2 years ago

I've made the requested changes.

One thing that I noticed is that the string form of UserRole is the string that was requested is stakeholdermanager but the enum ManagerStakeholder, please let me know if I should change one of them to the other to avoid confusion

Zshan0 commented 2 years ago

Since there is a change of StakeholderManager from ManagerStakeholder, should it have its own commit? There is an interleaving between the commits that are required in the same squash, which can't be done I think. So what do you propose the final list of squashed commits be?

darosior commented 2 years ago

Since there is a change of StakeholderManager from ManagerStakeholder, should it have its own commit?

I think so, yes.

So what do you propose the final list of squashed commits be?

I think the best one would be the refactoring first, then the new feature.

Also -and i forgot this in my previous comment- any new feature needs tests. Given it's a small thing here, no need for it to be too elaborated. A simple addition to the sanity check for this RPC command in the functional tests would do: https://github.com/revault/revaultd/blob/d014e662a5b3ace996362bfd2ba63fd1b6fad8ea/tests/test_rpc.py#L23-L33

darosior commented 2 years ago

Needs rebase now that #384 is merged. Thanks for adding a test. This still needs commits to be squashed: a6ac762d8501975248ee2465693457dab8a3ea2d and 9a5dbf703f4824cbf6425ccdc7198938eba10005 should be merged together.

Zshan0 commented 2 years ago

This PR can be closed since there is a more updated one with commits arranged as required

darosior commented 2 years ago

Please not for next time to not duplicate pull requests, you can just force-push to this one.