Feature/trezor common #1838
Feature/trezor common #1838
Conversation
OBorce
requested review from
erubboli,
azarovh and
ImplOfAnImpl
as code owners
OBorce
force-pushed
the
feature/trezor-common
branch
2 times, most recently
from
c2c31bb to
a99237c
Compare
OBorce
force-pushed
the
feature/trezor-common
branch
2 times, most recently
from
00f5576 to
44b17f9
Compare
| impl From<chain::OutPointSourceId> for crate::OutPointSourceId { | ||
| fn from(value: chain::OutPointSourceId) -> Self { | ||
| match value { | ||
| chain::OutPointSourceId::Transaction(tx) => Self::Transaction(tx.to_hash().into()), | ||
| chain::OutPointSourceId::BlockReward(tx) => Self::BlockReward(tx.to_hash().into()), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl From<chain::UtxoOutPoint> for crate::UtxoOutPoint { | ||
| fn from(value: chain::UtxoOutPoint) -> Self { |
There was a problem hiding this comment.
Though I don't insist on this, I'd suggest to make the make_random... functions return a pair of values that are supposed to be encoded to the same bytes, and remove these From implementations (which look somewhat fragile to me).
There was a problem hiding this comment.
I think it is the same, because you would still need to put the same random data in the right fields for both versions when generating them.
wallet/trezor-common/src/lib.rs
Outdated
| #[derive(FromPrimitive)] | ||
| pub enum AccountCommandIndex { | ||
| MintTokens = 0, | ||
| UnmintTokens = 1, | ||
| LockTokenSupply = 2, | ||
| FreezeToken = 3, | ||
| UnfreezeToken = 4, | ||
| ChangeTokenAuthority = 5, | ||
| ConcludeOrder = 6, | ||
| FillOrder = 7, | ||
| ChangeTokenMetadataUri = 8, | ||
| } |
There was a problem hiding this comment.
Is this enum needed here? As I can see, it's only used in the trezor firmware repo in encode_token_account_command_input but only on the rust side. And its values are not tied to anything, they just have to be the same in rust and python. So probably it should go to messages-mintlayer.proto in the trezor repo, so that named constants can be used on the python side too.
wallet/trezor-common/src/lib.rs
Outdated
| #[derive(FromPrimitive)] | ||
| pub enum OutPointSourceIdIndex { | ||
| Transaction = 0, | ||
| BlockReward = 1, | ||
| } |
There was a problem hiding this comment.
As I can see, this is similar to AccountCommandIndex, but the difference is that there is already MintlayerUtxoType in the protobuf config which this enum should be consistent with. Remove it and use MintlayerUtxoType instead.
OBorce
force-pushed
the
feature/trezor-common
branch
2 times, most recently
from
e9d69d2 to
c57154a
Compare
trezor-common/src/lib.rs
Outdated
| } | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq, Encode, Decode, EnumDiscriminants)] | ||
| #[strum_discriminants(name(OutputTimeLockIndex), derive(EnumIter, FromPrimitive))] |
There was a problem hiding this comment.
As I can see, OutputTimeLockIndex has nothing to do with OutputTimeLock, same as it was for AccountCommandIndex and OutPointSourceIdIndex. But it must be conmsistent with lock_type returned by get_lock in the trezor repo. So it makes sense to do the same for it as well - move it to rust code in the trezo repo, add same enum to the .proto file, write tests that ensures their consistency.
P.S. probably these extra protobuf enums should go to a separate file, because they have nothing to do with the protocol and only serve as a way to have the same constants in python and rust.
trezor-common/src/tests/mod.rs
Outdated
| } | ||
|
|
||
| fn make_random_account_command(rng: &mut (impl Rng + CryptoRng)) -> chain::AccountCommand { | ||
| match crate::AccountCommandIndex::iter().choose(rng).unwrap() { |
There was a problem hiding this comment.
Btw, the idea was that you add Tag enum to chain::AccountCommand so that if it gets more variants, we get a compilation failure here. Same for other "XXXIndex" enums.
Also, I suggest calling them "XXXTag", because "index" is a bit confusing IMO. I.e. we also have those #[codec(index = n)] annotations on the "parent" enums, which may not be consecutive, and "XXXIndex" sounds like at has something to do with those indices. And "XXXTag" sounds like it's just a name.
There was a problem hiding this comment.
It is hard to re-export the AccountCommandTag from the original as it would bring a dependency on that crate, and it is not nostd compliant. As for the handling of new variants it will be caught by the From implementations, they do a match on the original enums.
I replaced Index with Type as it sounds better to me, AccountCommandType than Tag?
There was a problem hiding this comment.
It is hard to re-export the AccountCommandTag from the original as it would bring a dependency on that crate, and it is not nostd compliant.
But it's for tests only, which can use std, right? I mean, you already use chain::DestinationTag, which also comes from a crate that is not nostd compliant.
As for the handling of new variants it will be caught by the From implementations, they do a match on the original enums.
Yeah, I guess it will work. But IMO it'd still be a bit nicer if the match was over Tag types (or whatever they will be called) that come with the type that is being generated. Otherwise it's not obvious that the test actually generates all possible "chain::AccountCommand"s
I replaced Index with Type as it sounds better to me, AccountCommandType than Tag?
Doesn't Type sound ambiguous?
In any case, we should probably use the same suffix for all such discriminant types. I.e. if "tag" is not good enough, chain::DestinationTag should be renamed too (I can do it myself or you can do it).
I'm not sure Type is a good alternative though. "Discriminant" might be better, but it's also longer.
There was a problem hiding this comment.
They are used in the Trezor firmware repo. On the python side we have the enums from the proto files, but on the rust side we use this enums. I added tests to make sure both of the enums correspond to each other.
I guess we can use Tag for consistency and it being short.
OBorce
force-pushed
the
feature/trezor-common
branch
from
e7610ee to
9fcfb4b
Compare
OBorce
force-pushed
the
feature/trezor-common
branch
2 times, most recently
from
c9cebcc to
d8de7c1
Compare
OBorce
force-pushed
the
feature/trezor-common
branch
from
d8de7c1 to
ebc5349
Compare
Move the simplified no-std types used by Trezor firmware to keep it in sync with the core types.
Add randomized tests to check the encodings match between the types.