helium / helium-ledger-app

The official Helium app for the Ledger Nano S
Apache License 2.0
32 stars 8 forks source link

"--new/old-address" is confusing since both validators and owners are addresses #46

Closed jamiew closed 2 years ago

jamiew commented 3 years ago

I think using --new-validator and --old-validator might be more clear

lthiery commented 3 years ago

I don't disagree but we will want to raise this with @madninja too. I've kept the ledger-app consistent with the UX of helium-wallet as much as possible and if I make the change here we probably want to carry it over to helum-wallet too:

https://github.com/helium/helium-wallet-rs/blob/master/src/cmd/validators/transfer.rs#L19

I think the logic on @madninja 's side has been to maintain the variables defined in the protobufs:

https://github.com/helium/proto/blob/master/src/blockchain_txn_transfer_validator_stake_v1.proto#L5

jamiew commented 3 years ago

Maybe allow for both but have docs use --new-validator? Don't love that solution either

lthiery commented 3 years ago

Yea that approach sounds like a half measure to me.

IMO, if we think it’s a worthwhile change, we break the interface but should maintain consistency with wallet.

madninja commented 3 years ago

Ugh.. Yeah I was trying to remain consistent with what's in the transaction. Both of the proposed options sound like more work than they're worth?

jamiew commented 3 years ago

I don't disagree. Feel free to close as #willnotfix. May god have mercy on our souls

lthiery commented 2 years ago

willnotfix :P