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

Stellar: add support for StellarManageBuyOfferOp and StellarPathPaymentStrictSendOp. #1838

Merged
merged 6 commits into from
Nov 4, 2021
Merged

Stellar: add support for StellarManageBuyOfferOp and StellarPathPaymentStrictSendOp. #1838

merged 6 commits into from
Nov 4, 2021

Conversation

overcat
Copy link
Contributor

@overcat overcat commented Oct 7, 2021 •
edited
Loading

Change List:

  • rename StellarManageOfferOp to StellarManageSellOfferOp
  • rename StellarPathPaymentOp to StellarPathPaymentStrictReceiveOp
  • rename StellarCreatePassiveOfferOp to StellarCreatePassiveSellOfferOp
  • add support for StellarManageBuyOfferOp
  • add support for StellarPathPaymentStrictSendOp
  • since the firmware does not support MuxedAccount, we refuse to process this type of transaction in trezorctl.
  • bump stellar-sdk to 5.0.0, trezorctl is not affected by the breaking changes in it, it's ok to bump it.

Note: This PR is based on the work of @MisterTicot. #627, #579

@overcat
Copy link
Contributor Author

overcat commented Oct 7, 2021

Hi @matejcik, I forgot to mention that CreatePassiveOfferOp was modified to CreatePassiveSellOfferOp in XDR, so I want to rename StellarCreatePassiveOfferOp to StellarCreatePassiveSellOfferOp, do you agree?

@matejcik matejcik self-requested a review October 7, 2021 14:53
@matejcik
Copy link
Contributor

matejcik commented Oct 7, 2021

Hi @matejcik, I forgot to mention that CreatePassiveOfferOp was modified to CreatePassiveSellOfferOp in XDR, so I want to rename StellarCreatePassiveOfferOp to StellarCreatePassiveSellOfferOp, do you agree?

yes, while we're at this :)

@overcat overcat marked this pull request as ready for review October 8, 2021 06:47
@overcat overcat requested a review from prusnak as a code owner October 8, 2021 06:47
@overcat
Copy link
Contributor Author

overcat commented Oct 8, 2021

Hi @matejcik, the PR is ready for review, if I am missing something, please let me know, thank you :-)

@prusnak prusnak removed their request for review October 8, 2021 18:23
@matejcik
Copy link
Contributor

just to let you know, this is in the pipeline but I haven't had time to review yet

@overcat
Copy link
Contributor Author

overcat commented Oct 14, 2021

just to let you know, this is in the pipeline but I haven't had time to review yet

Got it :-)

matejcik
matejcik approved these changes Nov 4, 2021
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.

very nice, i could only find one typo.

please fix that, rebase on master and fix conflicts

core/src/apps/stellar/operations/serialize.py Outdated Show resolved Hide resolved
@matejcik matejcik mentioned this pull request Nov 4, 2021
Closed
@matejcik
Copy link
Contributor

matejcik commented Nov 4, 2021

waiting for the CI pipeline

would you mind doing a corresponding PR to Trezor Connect?

@overcat
Copy link
Contributor Author

overcat commented Nov 4, 2021 •
edited
Loading

would you mind doing a corresponding PR to Trezor Connect?

@matejcik, I will submit a PR as soon as possible, maybe tomorrow.

@matejcik matejcik merged commit 3af00f4 into trezor:master Nov 4, 2021
@overcat
Copy link
Contributor Author

overcat commented Nov 4, 2021

Hi @matejcik, will the changes here be automatically synced to trezor-common? If not, can you please sync it? Thanks.

@matejcik
Copy link
Contributor

matejcik commented Nov 4, 2021

@overcat done

@matejcik matejcik mentioned this pull request Nov 4, 2021
Closed
@overcat overcat mentioned this pull request Nov 5, 2021
Merged
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

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

Successfully merging this pull request may close these issues.
2 participants
@overcat @matejcik