Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(common & core & legacy & python/stellar): add support for ClaimClaimableBalanceOp. #3434

Merged
merged 1 commit into from
Dec 11, 2023
Merged

feat(common & core & legacy & python/stellar): add support for ClaimClaimableBalanceOp. #3434

merged 1 commit into from
Dec 11, 2023

Conversation

overcat
Copy link
Contributor

@overcat overcat commented Dec 1, 2023 •
edited
Loading

Closes #3433

It has been two years since I last submitted code. I hope I haven't missed anything. If there is, please let me know. Thank you 🙏

@overcat overcat force-pushed the stellar/claim-claimable-balance branch 2 times, most recently from 3f02344 to 6946c7e Compare December 1, 2023 11:08
@overcat overcat marked this pull request as ready for review December 1, 2023 11:10
prusnak
prusnak previously requested changes Dec 3, 2023
View reviewed changes
python/src/trezorlib/stellar.py Outdated Show resolved Hide resolved
core/src/apps/stellar/operations/layout.py Outdated Show resolved Hide resolved
core/src/apps/stellar/operations/layout.py Outdated Show resolved Hide resolved
legacy/firmware/protob/messages-stellar.options Outdated
@@ -51,5 +51,8 @@ StellarManageDataOp.value max_size:65

StellarBumpSequenceOp.source_account max_size:57

StellarClaimClaimableBalanceOp.source_account max_size:57
StellarClaimClaimableBalanceOp.balance_id max_size:36
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs extra byte

Suggested change
StellarClaimClaimableBalanceOp.balance_id max_size:36
StellarClaimClaimableBalanceOp.balance_id max_size:37

Copy link
Contributor Author

@overcat overcat Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need to add an extra byte here because what is passed in is raw bytes (like memo_hash), not a string.

python/tests/test_stellar.py Outdated Show resolved Hide resolved
@overcat overcat requested a review from prusnak December 4, 2023 03:21
matejcik
matejcik reviewed Dec 7, 2023
View reviewed changes
Copy link
Contributor

@matejcik matejcik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please also regenerate the UI fixtures for model R (export TREZOR_MODEL=R before building core)

i'll need to confirm whether we'll support further Stellar changes to the legacy (trezor one) codebase

common/protob/messages.proto Outdated Show resolved Hide resolved
@matejcik
Copy link
Contributor

matejcik commented Dec 7, 2023

i'll need to confirm whether we'll support further Stellar changes to the legacy (trezor one) codebase

for now please remove the change from legacy. you will need to add the message name to legacy/firmware/protob/Makefile for exclusion.

please squash your fixups and rebase to get rid of the conflict

@overcat overcat force-pushed the stellar/claim-claimable-balance branch 2 times, most recently from 23bb42a to 590d454 Compare December 7, 2023 15:11
@overcat
Copy link
Contributor Author

overcat commented Dec 7, 2023

i'll need to confirm whether we'll support further Stellar changes to the legacy (trezor one) codebase

for now please remove the change from legacy. you will need to add the message name to legacy/firmware/protob/Makefile for exclusion.

please squash your fixups and rebase to get rid of the conflict

Done, PTAL.

@quietbits quietbits mentioned this pull request Dec 7, 2023
Merged
@matejcik
Copy link
Contributor

matejcik commented Dec 7, 2023

waiting for CI to go green and to see the new tests

(failed changelog check is OK, technically you could have put the legacy change in a separate commit and put a [no changelog] tag in the commit message, but it's not really important)

@overcat
Copy link
Contributor Author

overcat commented Dec 7, 2023

Hi @matejcik, we also need to skip the relevant tests on t1. I have added it in 42f9d72. Please let me know if there is a better way.

@overcat overcat force-pushed the stellar/claim-claimable-balance branch from 42f9d72 to ccf2f1b Compare December 8, 2023 12:00
@overcat
Copy link
Contributor Author

overcat commented Dec 8, 2023

It looks like we have passed the necessary tests, so I performed a rebase.

@overcat overcat force-pushed the stellar/claim-claimable-balance branch from ccf2f1b to c62f84f Compare December 8, 2023 12:07
@matejcik
Copy link
Contributor

matejcik commented Dec 8, 2023

Hi @matejcik, we also need to skip the relevant tests on t1. I have added it in 42f9d72. Please let me know if there is a better way.

seems reasonable, thank you.

one last CI pass for the final version of the commit

@matejcik matejcik dismissed prusnak’s stale review December 11, 2023 10:14

all is resolved

@matejcik matejcik merged commit 579cc0d into trezor:main Dec 11, 2023
55 of 56 checks passed
@matejcik
Copy link
Contributor

and we're in. thanks for your work 💯

@overcat
Copy link
Contributor Author

overcat commented Dec 11, 2023

and we're in. thanks for your work 💯

@matejcik Thank you for your support. Additionally, I would like to ask if the reason we no longer add new features to t1 is due to limitations in t1's performance? For example, insufficient Flash space?

@matejcik
Copy link
Contributor

It's more of a question of maintenance load. With the Safe 3 out (same codebase as the TT), we are shifting T1 into maintenance mode, meaning we will keep it working but not add new features. This one is very small, sure, but we need to draw the line somewhere.

@overcat overcat deleted the stellar/claim-claimable-balance branch December 11, 2023 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matejcik matejcik matejcik approved these changes

@prusnak prusnak Awaiting requested review from prusnak prusnak is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Stellar: add support for ClaimClaimableBalanceOp.
3 participants
@overcat @matejcik @prusnak