Skip to content

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

Merged
merged 43 commits into from
Mar 5, 2025
Merged

Feature/mintlayer pk #1

merged 43 commits into from
Mar 5, 2025

Conversation

OBorce
Copy link
Collaborator

@OBorce OBorce commented May 15, 2024

Add new messages in the proto files for Mintlayer coin support, and handlers.

@OBorce OBorce force-pushed the feature/mintlayer-pk branch from bddbd25 to 967640b Compare July 16, 2024 09:08
@OBorce OBorce force-pushed the feature/mintlayer-pk branch from 5abd648 to 798be63 Compare August 6, 2024 09:54
@azarovh
Copy link

azarovh commented Aug 6, 2024

I wouldn't merge this to master but rather created a separate branch like mintlayer-master and merge features there. This way we can keep it clean and isolated and when we decide to update trezor version we will pull to master and rebase our master. Otherwise it could become a mess very quickly. Makes sense?

One thing to consider is that CI might not work for "our" master.

Copy link
Collaborator

@ImplOfAnImpl ImplOfAnImpl left a 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.

@OBorce OBorce force-pushed the feature/mintlayer-pk branch 16 times, most recently from 941d641 to dd3f1e8 Compare September 12, 2024 16:29
@OBorce OBorce changed the base branch from master to main September 12, 2024 16:32
@OBorce OBorce force-pushed the feature/mintlayer-pk branch 9 times, most recently from 08dce8d to 77bb6af Compare September 16, 2024 23:28
@OBorce OBorce force-pushed the feature/mintlayer-pk branch from 04bd932 to 4c5d084 Compare February 9, 2025 16:01
@OBorce OBorce force-pushed the feature/mintlayer-pk branch from b86b574 to e9d6bf2 Compare February 10, 2025 08:25
Copy link
Collaborator

@ImplOfAnImpl ImplOfAnImpl left a 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.

  1. Some older review comments have not been addressed. Plz make sure all the comments are addressed before merging.
  2. The build is broken after the latest rebase.

return client.call(
messages.MintlayerGetAddress(
address_n=address_n,
chain_type=messages.MintlayerChainType(chain_type),
Copy link
Collaborator

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.

Copy link
Collaborator Author

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

Comment on lines +325 to +331
/** 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
}
Copy link
Collaborator

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).

@OBorce OBorce force-pushed the feature/mintlayer-pk branch 4 times, most recently from 511c8de to 4869c88 Compare February 17, 2025 08:50
@OBorce OBorce force-pushed the feature/mintlayer-pk branch from 4869c88 to bf2afd0 Compare February 18, 2025 09:47
@OBorce OBorce force-pushed the feature/mintlayer-pk branch from 7307ceb to 77fc7d7 Compare March 5, 2025 06:53
@OBorce OBorce force-pushed the feature/mintlayer-pk branch from 77fc7d7 to 04f7132 Compare March 5, 2025 07:07
@OBorce OBorce merged commit 75f4d84 into mintlayer-master Mar 5, 2025
16 checks passed
@ImplOfAnImpl ImplOfAnImpl mentioned this pull request Mar 5, 2025
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants