aeternity / AEXs

Aeternity expansions repository — application layer standards
10 stars 25 forks source link

AEX-2: In what cases aepp may need to disallow wallet to change transaction? #60

Closed davidyuk closed 4 years ago

davidyuk commented 5 years ago

Or what is the purpose of locked flag in aepp.request.sign params? (related issue: #55)

shekhar-shubhendu commented 5 years ago

as you can see the default value of locked is false so this should not be a concern for the base-app.

but nevertheless since you've asked, this has been put as mechanism for apps that would not like the users to update the created transactions therefore they can indicate this using this locked field. E.g. an app that handles state channel txs would not want wallet or users to update the txs created by the node.

davidyuk commented 5 years ago

A possibility of set locked: true will make Base app to implement its support.

I see that this flag does. Why app that handles state channel may not want a wallet to update transaction?

If a wallet has several signing requests at the same time (apps can't foresee this so they send transactions with the same nonce), in that case, the wallet can set nonces in order of confirmations by itself, usage of locked: true will break this.

shekhar-shubhendu commented 5 years ago

I see that this flag does. Why app that handles state channel may not want a wallet to update transaction?

There are certain fields in the state channel tx and most of the time the tx are created by the node and should not be changed by the user.

If a wallet has several signing requests at the same time (apps can't foresee this so they send transactions with the same nonce), in that case, the wallet can set nonces in order of confirmations by itself, usage of locked: true will break this.

is this the only case/field you're worried about. if you want we can add an exception for this in the aex.

davidyuk commented 5 years ago

There are certain fields in the state channel tx

What fields? Can you be more specific?

if you want we can add an exception for this in the aex.

I want to have removed all unnecessary parts of AEX-2 like this one to reduce its overcomplexity.

shekhar-shubhendu commented 5 years ago

What fields? Can you be more specific?

Example: channel_create tx. this defines the list of fields that both the parties need to agree on for the state channel creation and therefore both party will have to sign it. if one party modifies the field on receiving a transaction the state channel will not be created.

I want to have removed all unnecessary parts of AEX-2 like this one to reduce its overcomplexity.

please keep things towards more constructive side.

is this the only case/field you're worried about. if you want we can add an exception for this in the aex.

please provide a list of fields that you think should be outside the purview of locked method. else this issue will be resolved without them and considered resolved.

davidyuk commented 5 years ago

please provide a list of fields that you think should be outside the purview of locked method. else this issue will be resolved without them and considered resolved.

Ok, it should be at least: fee, nonce. Looks more reasonable to replace locked flag with a list of fields and transaction type that can be edited by wallet. Otherwise, aepp developer should set locked: true even for spend transactions to ensure that wallet won't change receiver address, as far as AEX-2 allows wallet to change any field if locked: false.

shekhar-shubhendu commented 5 years ago

@davidyuk yes. actually i was also thinking of something like that. will make the appropriate changes and let you know.

AndreasGassmann commented 5 years ago

I think the last idea discussed with the list of "locked" fields makes most sense so far.

However, from a security perspective, we can never assume that the wallet plays by the rules and doesn't just simply ignore the "locked" fields. So fields like amount, fee and recipient (maybe more?) should always be validated on the aepp / user side anyway.

So I'm not sure if this flag makes sense at all. Because it looks like it's meant as a security feature, but in the end it doesn't improve security at all. The user / aepp dev might be under the false impression that these fields are really locked, but in the end they can still be overwritten.

shekhar-shubhendu commented 5 years ago

@AndreasGassmann it was never meant to be used as a security feature but more of an indication to the wallet that it can modify the tx as required or not.

AndreasGassmann commented 5 years ago

Is there a "list" of reasons why a wallet would ever need to change a transaction?

From this discussion I can see that one of them is the nonce that might be edited if there are many transactions, and maybe the fee that needs to be adjusted.

But to be honest, if I call a method called aepp.request.sign, I would never ever expect the other side to alter my request in any significant way and sign something different. I would expect my exact string/transaction to be signed. If that is not possible for any reason, I would expect an error (eg. Nonce too low, Fee too low, etc).

This feels more like a payment/contract request where you basically provide something that you want to happen (eg. send x amount of AE, some contract call), and then the wallet handles the rest and prepares the whole transaction with the parameters that make most sense at that time (fee, nonce, etc).

If it's the second then that makes sense (also the locked flag), then it's just the method name that is a bit confusing to me.

That being said, if you both of agree that the name should be kept, I'm ok with it.

shekhar-shubhendu commented 4 years ago

the AEX will be reverted to use locked flag instead of a list. it would be made optional to honor the field but aepp can use it to signal its intentions.

noandrea commented 4 years ago

still on debate. the locked flag will be removed from the aex-2.

davidyuk commented 4 years ago

fixed in #118