-
Notifications
You must be signed in to change notification settings - Fork 0
Feature/mintlayer pk #1
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
Conversation
bddbd25
to
967640b
Compare
5abd648
to
798be63
Compare
I wouldn't merge this to master but rather created a separate branch like One thing to consider is that CI might not work for "our" master. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I can see, there are still many FIXMEs and commented lines, so it seems like this is still work-in-progress.
core/embed/extmod/modtrezormintlayer/modtrezormintlayer-utils.h
Outdated
Show resolved
Hide resolved
941d641
to
dd3f1e8
Compare
08dce8d
to
77bb6af
Compare
04bd932
to
4c5d084
Compare
b86b574
to
e9d6bf2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably this should be merged after the relatively trivial comments have been aplied. And the rest can be addressed via separate tasks. But let's create an issue with a list of such tasks, so that they are not lost.
P.S.
- Some older review comments have not been addressed. Plz make sure all the comments are addressed before merging.
- The build is broken after the latest rebase.
return client.call( | ||
messages.MintlayerGetAddress( | ||
address_n=address_n, | ||
chain_type=messages.MintlayerChainType(chain_type), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will raise an exception like "ValueError: '1' is not a valid MintlayerChainType" because chain_type
is a string.
So, it should be messages.MintlayerChainType(int(chain_type))
instead.
Same for other commands below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed the CLI to parse the arguments as int
/** Data type for transaction Transfer output to be signed. | ||
* @embed | ||
*/ | ||
message MintlayerTransferTxOutput { | ||
required string address = 1; // destination address in bech32 encoding | ||
required MintlayerOutputValue value = 2; // amount to spend in atoms for coin or token | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For bitcoin, they have an address_n
field which is used for change outputs. So the device knows when an output is a change one and doesn't show the output confirmation to the user in such a case.
IMO we should behave similarly, because confirming change outputs makes no sense and it looks kind of scary (the user may only guess that the address is a change one and that it actually belongs to them).
P.S. Probably this should be addressed in a separate task after this PR is merged (though it will require a change int he protocol).
Let's maybe create an issue for this. so that it doesn't get lost (or may be we should have a single issue with the list of things to do).
511c8de
to
4869c88
Compare
4869c88
to
bf2afd0
Compare
7307ceb
to
77fc7d7
Compare
77fc7d7
to
04f7132
Compare
Add new messages in the proto files for Mintlayer coin support, and handlers.