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.

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

All: add Stellar operation manageBuyOffer #627

Open
wants to merge 3 commits into
base: master
from

Conversation

@MisterTicot
Copy link

MisterTicot commented Oct 16, 2019

I think I covered everything but tests: tests for similar operations manageOffer/createPassiveOffer were missing too.

I did not test the implementation as I don't know about how to proceed. Please let me know if anything is missing or wrong.

@tsusanka

This comment has been minimized.

Copy link
Contributor

tsusanka commented Oct 16, 2019

Please run pipenv run make gen to generate protobuf messages.


Yes, you are right that some tests for Stellar operations are missing (see #2). These device tests however are a great start to test your implementation. Have a look on the first test for example and try to write a test for manageBuyOffer, it should be very similar. There is a comment on the top of the file explaining how to get a fixture from Stellar's labrotory.

@MisterTicot

This comment has been minimized.

Copy link
Author

MisterTicot commented Oct 16, 2019

@tsusanka Can you take it from here? I feel like I'm wasting huge amount of time trying to figure out something that you could solve in a dozen of minutes. Meanwhile, nobody is busy writing tests & updates for my own softwares.

@tsusanka

This comment has been minimized.

Copy link
Contributor

tsusanka commented Oct 16, 2019

What is the difference between StellarManageBuyOfferOp and the StellarManageOfferOp we already had? Is it completely unrelated op? Or do we use StellarManageOfferOp for "sell"?

@matejcik

This comment has been minimized.

Copy link
Contributor

matejcik commented Oct 17, 2019

What is the difference between StellarManageBuyOfferOp and the StellarManageOfferOp we already had? Is it completely unrelated op? Or do we use StellarManageOfferOp for "sell"?

Correct, "manage offer" is old name for "manage sell offer". We might want to rename "manage offer" to avoid confusion, however, it would break existing Connect users.
OTOH there probably aren't any, so ¯\_(ツ)_/¯.

Another thing i notice is that the messages are identical in terms of fields. The same code could handle them, and we could even fold them into a single message with a BUY/SELL enum flag. Leaving aside the fact that this is what Stellar devs should have done in the first place, this is where the atypical way Stellar is implemented is biting us in the behind.

I mean
we can make three distinct protobuf messages with the same fields (CreatePassiveOffer also has identical fields, except offer id is missing - but that's optional anyway so again ¯\_(ツ)_/¯)
but we should reuse as much existing ManageOffer implementation as possible.
and that is like 98 % of it.

@MisterTicot

This comment has been minimized.

Copy link
Author

MisterTicot commented Oct 18, 2019

Correct, "manage offer" is old name for "manage sell offer". We might want to rename "manage offer" to avoid confusion, however, it would break existing Connect users.
OTOH there probably aren't any, so ¯_(ツ)_/¯.

stellaport.io is one of the major DEX front-end and has integrated Trezor since nearly one year. Naturally, they use the manage(Sell)Offer operation - so I'd assume there are people using it.

@MisterTicot

This comment has been minimized.

Copy link
Author

MisterTicot commented Oct 24, 2019

Note: the latest HEAD passed the checks locally, but when I build T firmware & try to sign createPassiveOffer/manageSellOffer/manageBuyOffer ops I get Firmware error. I've tried to rebase on trezor-firmware master but got the same error.

MisterTicot added 3 commits Oct 16, 2019
@MisterTicot MisterTicot force-pushed the MisterTicot:stellar-manage-buy-offer branch from 6f14b8e to 59b07c8 Oct 25, 2019
@MisterTicot

This comment has been minimized.

Copy link
Author

MisterTicot commented Oct 25, 2019

Pull request rebased

Example of transactions leading to firmware error: manageSellOffer.

@MisterTicot

This comment has been minimized.

Copy link
Author

MisterTicot commented Nov 1, 2019

Anybody here???

@prusnak prusnak added this to the backlog milestone Nov 10, 2019
@MisterTicot

This comment has been minimized.

Copy link
Author

MisterTicot commented Nov 25, 2019

I spent time to play your game: learn how to use your development environment & write some missing code in Trezor support for Stellar. I had to figure out by myself how to make it fits your system.

Yet that PR is being ignored since a month. This is totally unrespectful. 😠

@matejcik

This comment has been minimized.

Copy link
Contributor

matejcik commented Nov 25, 2019

hello, you still haven't addressed this comment #627 (comment):

Another thing i notice is that the messages are identical in terms of fields. The same code could handle them
(...)
but we should reuse as much existing ManageOffer implementation as possible.
and that is like 98 % of it.

I am sorry that i don't have more time to spend on 3rd party contributions in order to provide more detailed guidance. However, this is a low-priority issue for us.

@MisterTicot

This comment has been minimized.

Copy link
Author

MisterTicot commented Nov 25, 2019

That issue was already present in your code. As you noted, there was already a duplicate for passiveSellOffer - I simply followed your ways.

I think there's a difference between me writing an update for your firmware & me refactoring your code base to fix a weakness in it. I'm not proficient at C & I never proposed to go that far.

I agree that this would be better but I fear that if you really want it then you'll have to write it by yourself.

optional string source_account = 1; // (optional) source account address
optional StellarAssetType selling_asset = 2;
optional StellarAssetType buying_asset = 3;
optional sint64 buy_amount = 4;

This comment has been minimized.

Copy link
@matejcik

matejcik Nov 25, 2019

Contributor

please rename to amount

This comment has been minimized.

Copy link
@MisterTicot

MisterTicot Dec 3, 2019

Author

You already asked for it the last time then closed it after I replied:

The reason I used buyAmount is because it's what Stellar use (I guess it should be buy_amount by the way?). I know it's not consistent.

Do you want to switch to amount anyway?

I think it's better to stay consistent with the underlying implementation regardless of our opinion about it. (I asked them to change it into amount at a lower level when that operation got merged but they didn't)

@@ -110,6 +123,19 @@
await confirm_asset_issuer(ctx, op.buying_asset)


async def _confirm_buy_offer(ctx, title, op):

This comment has been minimized.

Copy link
@matejcik

matejcik Nov 25, 2019

Contributor

please extend and reuse _confirm_offer instead

This comment has been minimized.

Copy link
@MisterTicot

MisterTicot Dec 3, 2019

Author

Looking into it, _confirm_offer and _confirm_buy_offer have few lines in common (4 out of 8). Whatever way you put it, the relation between buying asset & selling asset is inversed - so is the price - so I see no way to rewrite _confirm_offer without crippling code readability.

@@ -68,6 +69,15 @@ def write_manage_offer_op(w, msg: StellarManageOfferOp):
writers.write_uint64(w, msg.offer_id)


def write_manage_buy_offer_op(w, msg: StellarManageBuyOfferOp):

This comment has been minimized.

Copy link
@matejcik

matejcik Nov 25, 2019

Contributor

please reuse write_manage_offer_op instead

This comment has been minimized.

Copy link
@MisterTicot

MisterTicot Dec 3, 2019

Author

I'm waiting for your response about amount so I can decide how to rewrite that one.

@matejcik

This comment has been minimized.

Copy link
Contributor

matejcik commented Nov 25, 2019

I pointed out some places where reuse should happen. You're unfortunately right about the legacy code, where we can't use duck typing to save on copy-pasting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.