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

⚗️ Actions returned from HTTP should be self-descriptive #494

Closed
LLFourn opened this Issue Nov 27, 2018 · 13 comments

Comments

Projects
None yet
4 participants
@LLFourn
Copy link
Contributor

LLFourn commented Nov 27, 2018

Decide how to implement this.

DoD:

  • Proposal on how to do it for every (type of) action. See comments in this issue.

When deserialization the bitcoin htlc funding action we currently get:

{
    address: 'bcrt1qf0qqxcsqka64cpnug74u4n5jz32tr6qjq7essrd5agvzw4mjlcaq6zrmeg',
    value: '100000000'
}

This should include a field saying what you're meant to do:

{
    type: "send-to-bitcoin-address",
    address: 'bcrt1qf0qqxcsqka64cpnug74u4n5jz32tr6qjq7essrd5agvzw4mjlcaq6zrmeg',
    value: '100000000'
}
@D4nte

This comment has been minimized.

Copy link
Member

D4nte commented Nov 27, 2018

Maybe better:

{
    ledger: 'Bitcoin'
    type: 'send-to-address'
    ...
}
@thomaseizinger

This comment has been minimized.

Copy link
Member

thomaseizinger commented Nov 27, 2018

We should also consider the possibility to use the Content-Type header of HTTP to describe what the content is that we are returning instead of putting it into the body.

So instead of Content-Type: application/json, we could send Content-Type: vendor/bitcoin-transaction+json. This would allow us to write more framework-like code to handle this stuff.

@LLFourn LLFourn added the open source label Dec 8, 2018

@LLFourn

This comment has been minimized.

Copy link
Contributor Author

LLFourn commented Dec 8, 2018

Note: I'm adding open source to this because it would make the json snippets in blog post nicer to read but team feels it's more of a "nice to have" so consider it to be done last.

@bonomat bonomat removed the open source label Dec 12, 2018

@D4nte D4nte changed the title Actions returned from HTTP should be self-descriptive ⚗️ Actions returned from HTTP should be self-descriptive Jan 16, 2019

@D4nte D4nte added groomed sprint-backlog and removed groomed labels Jan 16, 2019

@D4nte D4nte added this to the Sprint 6 🔄📖 milestone Jan 16, 2019

@thomaseizinger thomaseizinger self-assigned this Jan 16, 2019

@thomaseizinger

This comment has been minimized.

Copy link
Member

thomaseizinger commented Jan 16, 2019

Mediatype vs "inline"-typing

For simplicity reasons, I would say we go with "inline"-typing for now and ditch the mediatype approach.
If we find that this approach is not flexible enough, we can always deprecate this API and move to a mediatype based one.

Design

We currently have four different actions, I've outlined what they could look like below:

Send-to-bitcoin address action

(amount will be in sat)

{
	"type": "send-amount-to-bitcoin-address",
	"payload": {
		"to": "...",
		"amount": "5000000"
	}
}

broadcast signed bitcoin transaction action

{
	"type": "broadcast-signed-bitcoin-transaction",
	"payload": {
		"hex": "..."
	}
}

deploy ethereum contract

(amount will be in wei)

{
	"type": "deploy-ethereum-contract",
	"payload": {
		"data": "...",
		"amount": "...",
		"gas_limit": "..."
	}
}

invoke ethereum contract

{
	"type": "invoke-ethereum-contract",
	"payload": {
		"to": "...",
		"data": "...",
		"gas_limit": "..."
	}
}

Design rationale

  1. Have stable keys in the payload: Each action always has the "type" and "payload" keys. This makes it easy to deserialize.
  2. Each action only has a single type:
    a. If we have something like ledger and type it might suggest that certain combinations are valid even though we don't have that. For example, if there is one with ledger: bitcoin, type: send-to-address does the same thing exist for ledger: ethereum?
    b. Actions are can be unambigous and can avoid optional fields. This makes deserialization very predicatable and reduces runtime errors.
    c. This way, all possible actions can easily be enumerated and form a "flat" list.
@LLFourn

This comment has been minimized.

Copy link
Contributor Author

LLFourn commented Jan 16, 2019

👍 from me.

We had the idea while you were away to be on the side of providing more information rather than less. For example:

  • If we send to a P2WSH address we could tell the user the actual contract they are deploying (hex opcodes).

  • If calling a contract like ERC20 transferFrom we could tell them more stuff like what's the name of the function they're calling, what are the arguments they're calling it with.

Obviously this kind of information isn't needed to execute the action but could be useful in some kind of orthogonal decision making and debugging. It would be very useful for application developers to gain an understanding of what is actually happening.

🔧 minor suggestions:

  • By convention we could start the action type name with the ledger its associated i.e. ethereum-invoke-contract.
  • I'd expect invoke-ethereum-contract's payload field to be contract_address rather than to.
@D4nte

This comment has been minimized.

Copy link
Member

D4nte commented Jan 16, 2019

Good to me. Agree with starting action name with associated ledger. ie ethereum-*

@bonomat

This comment has been minimized.

Copy link
Member

bonomat commented Jan 17, 2019

One more thought on that: while some actions do not exist on all blockchains, we can try to generalize others. They are mostly similar and reduce the amount of different action types we need to hold in some enum, etc..

The question is what do you want to express with the type field: should it tell you what to do or should it tell you where you are in the process of an atomic swap using RFC003?

For example,

Deploy Ethereum smart contract

{
	"type": "send-to-address",
	"ledger": "ethereum"
	"payload": {
		"data": "...",
		"amount": "...",
		"gas_limit": "..."
		"address": "..."
	}
}

The same action could be used for invoke Ethereum smart contract.

The same action can also be used for Bitcoin, just leave out the Ethereum related fields, e.g. for

Funding Bitcoin HTLC:

{
	"type": "send-to-address",
	"ledger": "bitcoin"
	"payload": {
		"amount": "...",
		"address": "..."
	}
}
@thomaseizinger

This comment has been minimized.

Copy link
Member

thomaseizinger commented Jan 17, 2019

We had the idea while you were away to be on the side of providing more information rather than less. For example:

I can see how this might be useful, however, I'd also consider it to be a separate feature. Let's keep the scope small and build things like that as we need them.

By convention we could start the action type name with the ledger its associated i.e. ethereum-invoke-contract.

Good point, I will incorporate that.

One more thought on that: while some actions do not exist on all blockchains, we can try to generalize others. They are mostly similar and reduce the amount of different action types we need to hold in some enum, etc..

I went back and forth between something very similar to your solution and what I ended up presenting. The reason I didn't go for that one is because if you take the generalization all the way to the end, you end up with send-transaction:

{
	"type": "send-transaction",
	"ledger": "ethereum",
	"payload": {
        "...": "..."
	}
}

The main reason I didn't want to go for that is because I think there is value in being unambiguous here about what should be done and to reduce runtime errors. This mainly boils down to not having optional fields:

An optional field always leads to runtime decisions that need to be made by clients. Although in the end, all these actions result in transactions being made, I think there is value in allowing client developers to have assurance of what is done by a certain code path. My feeling would be that relying on the type field to determine the missing data for a transaction is less error-prone because it is more explicit than doing it based on missing fields. Take the gas_limit on Ethereum for example: If the action says send-ether-to-address, you know how much gas that is going to cost and therefore hardcode it at that particular code path. If the action just says, send-transaction and we only supply the value, the client developer would have to build some heuristics to figure out, how much gas they should supply.

In the end, I would prefer clients to be as dumb as possible because it reduces the risk of using the API in a wrong way.

If they are a fan of generalizing all these actions, they can always write a function themselves that normalizes all these parameters into a single function call which eventually sends a transaction. However, more specific types all them to very explicitly handle each action.

They are mostly similar and reduce the amount of different action types we need to hold in some enum, etc..

I had the same thought. However, in the end, there is not really much cost associated with it. Plus it allows us to specify all possible values at compile-time, which results in less runtime errors (compared to a combinatorial solution).

Here is the updated proposal:

Send-to-bitcoin address action

{
	"type": "bitcoin-send-amount-to-address",
	"payload": {
		"to": "...",
		"amount": "5000000"
	}
}

broadcast signed bitcoin transaction action

{
	"type": "bitcoin-broadcast-signed-transaction",
	"payload": {
		"hex": "..."
	}
}

deploy ethereum contract

{
	"type": "ethereum-deploy-contract",
	"payload": {
		"data": "...",
		"amount": "...",
		"gas_limit": "..."
	}
}

invoke ethereum contract

{
	"type": "ethereum-invoke-contract",
	"payload": {
		"contract_address": "...",
		"data": "...",
		"gas_limit": "..."
	}
}
@bonomat

This comment has been minimized.

Copy link
Member

bonomat commented Jan 17, 2019

Yap, makes sense.

Just one thing on

If the action says send-ether-to-address, you know how much gas that is going to cost and therefore hardcode it at that particular code path.

I would be careful with this as it is not generally true. If the target is a contract even the default function could have some logic in it making the transaction more expensive.
I'm saying this because it sounds like that you would use send-ether-to-address only for accounts but you cannot distinguish an account and a contract address.

In regards of the proposal:

  • to make it even more clear for invoking a contract on ethereum
    I would add `"amount" : 0: it, i.e.
{
	"type": "ethereum-invoke-contract",
	"payload": {
		"contract_address": "...",
		"data": "...",
		"gas_limit": "...",
		"amount": 0
	}
}
@LLFourn

This comment has been minimized.

Copy link
Contributor Author

LLFourn commented Jan 17, 2019

@bonomat

I would be careful with this as it is not generally true. If the target is a contract even the default function could have some logic in it making the transaction more expensive.

Hmmm. I thought about the situation where this might bite us. The only one I could think of is that a user supplies a contract address as their redeem/refund address thinking it will just work. Then when they reveal the secret or whatever they find that it's impossible to get their funds out.

I googled around and found out that this actually can't happen in our case because we use selfdestruct which transfers eth but doesn't do a call.

to make it even more clear for invoking a contract on ethereum I would add `"amount" : 0

For completeness sake I agree. There are contracts that have functions that require the user to transfer eth in the call so we should handle this case.

[EDIT] Also we may want to include the network if we are serious about doing this for Ethereum. It matters when you are executing the action obviously.

@thomaseizinger

This comment has been minimized.

Copy link
Member

thomaseizinger commented Jan 17, 2019

[EDIT] Also we may want to include the network if we are serious about doing this for Ethereum. It matters when you are executing the action obviously.

Network may be a good idea for all actions. Will add it!

to make it even more clear for invoking a contract on ethereum
I would add `"amount" : 0: it, i.e.

Good point! Will add it!

@thomaseizinger

This comment has been minimized.

Copy link
Member

thomaseizinger commented Jan 17, 2019

I'll consider this spike to be done with the creation of #673.

@bonomat

This comment has been minimized.

Copy link
Member

bonomat commented Jan 17, 2019

Hmmm. I thought about the situation where this might bite us. The only one I could think of is that a user supplies a contract address as their redeem/refund address thinking it will just work. Then when they reveal the secret or whatever they find that it's impossible to get their funds out.

I googled around and found out that this actually can't happen in our case because we use selfdestruct which transfers eth but doesn't do a call.

Do you have a link to that?

I would be interesting to see what will happen when you selfdescruct to another contract. I would assume that it will invoke the default function of that contract.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment