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

⚗️ More (debug) information returned by HTTP API GET swap/:id #497

Closed
D4nte opened this Issue Nov 28, 2018 · 16 comments

Comments

Projects
None yet
4 participants
@D4nte
Copy link
Member

D4nte commented Nov 28, 2018

Motivations:

  • debugging: debug blockchain without having to dig tx id, addresses in logs
  • transparency: allow user to check the tx, contract etc on the blockchain

Expose the following information in the http api:

  • transaction id
  • reject/decline reason
  • internal failure cause if known
  • addresses (HTLC)
  • identities

Hints:

  • The implementor should consider changing what is retrieved from the storage layer as per #576 .

Omit:

  • secret (could be added as a verbose feature at a later stage)
  • transient keys

Pre-work:
Grab a colleague and decide on the output format

DoD:

  • Information above (if known) is returned at each state of a swap
  • Make LQS go offline and create relevant info
  • Decline reasons are displayed

Child of #405.
Child of #399.
Blocked by #576.
Blocked by comit-network/RFCs#6.

@D4nte

This comment has been minimized.

Copy link
Member Author

D4nte commented Dec 16, 2018

To be reviewed once #576 is done.

@D4nte D4nte added the icebox label Dec 16, 2018

@D4nte

This comment has been minimized.

Copy link
Member Author

D4nte commented Dec 16, 2018

At first: put as much as possible in the http output
Then, can add a configuration option to protect sensitive data such as transit keys and secret.

@luckysori

This comment has been minimized.

Copy link
Contributor

luckysori commented Dec 19, 2018

As part of #590 Bob can decline a swap request providing a reason, but this information is currently not displayed in the response to a GET /swap/rfc003/:id request.

Suggestion:

Make the state an object instead of a string. This object can hold the name of the state and a set of extra fields which will vary depending on the state (for Rejected the reason would be one of them).

@D4nte D4nte changed the title Fully functional Swap details returned in get swaps More (debug) information returned by HTTP API Jan 6, 2019

@D4nte D4nte changed the title More (debug) information returned by HTTP API More (debug) information returned by HTTP API GET swap/:id Jan 6, 2019

@thomaseizinger thomaseizinger self-assigned this Jan 15, 2019

@wafflebot wafflebot bot added work-in-progress and removed groomed labels Jan 15, 2019

@thomaseizinger

This comment has been minimized.

Copy link
Member

thomaseizinger commented Jan 15, 2019

I did some brainstorming on what this could look like. The previous AlphaRedeemedBetaFunded state could be modeled like this:

{
	"state": {
  	"alpha_ledger": {
  		"current_state": "Redeemed",
  		"htlc_location": "37Ju7oTbPfe5qNAHYiAKfdTtVvCSP3TTdL",
  		"redeem_identity": "62E907B15CBF27D5425399EBF6F0FB50EBB88F18",
  		"refund_identity": "f3f2655c6de2a4c2cc00e58102252e940cc88772",
  		"funding_tx": "0x3027661dcac1a70e1ee19a20561b7e304698e52a1eae93fa340e2a1af4afd176",
  		"redeem_tx": "0x52dc163c5fbb1eef1921c21140008860558842c074eb79d3317d194fe0b773fb"
  	},
  	"beta_ledger": {
  		"current_state": "Funded",
  		"htlc_location": "0xccb882aa9d339b1af3ba01db82cf8ebb443c914c",
  		"funding_tx": "0xca7d9d05f474565dcc6f5fef0d8878d6d4e19d2cd8ba9e634e60d6658b879a9c"
  	}
  },
}

A couple of things to note:

  • alpha_ledger is Bitcoin

  • beta_ledger is Ethereum

  • identities are only included if they are transient

  • both objects use the same terminology (htlc_location, current_state, etc...)

  • the terminology is specific to RFC003, which I think is fine. Other swap protocols may return completely different data

  • state accumulates, i.e. the funding_tx is still there even though the state progressed to Redeemed

Is this roughly what you had in mind?

@LLFourn

This comment has been minimized.

Copy link
Contributor

LLFourn commented Jan 15, 2019

My mental picture was a bit different but I want to see where this one goes. I had request and response and then only have blockchain state related stuff in the alpha_ledger (perhaps nested in a setup object which tracks the state of the RFC003 setup phase).

Try and extend this description to include the assets. I'm wondering why some HTLC parameters are in the alpha_ledger thing but the alpha asset is not in there too. I guess these are RFC002 parameters so you include them on another layer.

@D4nte

This comment has been minimized.

Copy link
Member Author

D4nte commented Jan 16, 2019

@thomaseizinger who did you Brainstorm with?

@thomaseizinger

This comment has been minimized.

Copy link
Member

thomaseizinger commented Jan 16, 2019

Me, myself and I!

@D4nte

This comment has been minimized.

Copy link
Member Author

D4nte commented Jan 16, 2019

I'd suggest @thomaseizinger and @LLFourn have a chat and agree on the API :)

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

@thomaseizinger

This comment has been minimized.

Copy link
Member

thomaseizinger commented Jan 16, 2019

My mental picture was a bit different but I want to see where this one goes. I had request and response and then only have blockchain state related stuff in the alpha_ledger (perhaps nested in a setup object which tracks the state of the RFC003 setup phase).

Try and extend this description to include the assets. I'm wondering why some HTLC parameters are in the alpha_ledger thing but the alpha asset is not in there too. I guess these are RFC002 parameters so you include them on another layer.

Yes, please note that this is just the relevant part that changes to the current output. Here is a full response:

{
  "_links": {
    "redeem": {
      "href": "/swaps/rfc003/ba111c00-672f-4710-9a0c-199abe26b9f3/redeem"
    }
  },
  "role": "Bob",
  "state": {
    "alpha_ledger": {
      "current_state": "Redeemed",
      "htlc_location": "37Ju7oTbPfe5qNAHYiAKfdTtVvCSP3TTdL",
      "redeem_identity": "62E907B15CBF27D5425399EBF6F0FB50EBB88F18",
      "refund_identity": "f3f2655c6de2a4c2cc00e58102252e940cc88772",
      "funding": {
        "tx": "0x3027661dcac1a70e1ee19a20561b7e304698e52a1eae93fa340e2a1af4afd176"
      },
      "redeem": {
        "tx": "0x52dc163c5fbb1eef1921c21140008860558842c074eb79d3317d194fe0b773fb"
      }
    },
    "beta_ledger": {
      "current_state": "Funded",
      "htlc_location": "0xccb882aa9d339b1af3ba01db82cf8ebb443c914c",
      "funding": {
        "tx": "0xca7d9d05f474565dcc6f5fef0d8878d6d4e19d2cd8ba9e634e60d6658b879a9c"
      }
    }
  },
  "swap": {
    "alpha_asset": {
      "name": "Bitcoin",
      "quantity": "100000000"
    },
    "alpha_ledger": {
      "name": "Bitcoin",
      "network": "regtest"
    },
    "alpha_lock_duration": {
      "type": "blocks",
      "value": 144
    },
    "beta_asset": {
      "name": "Ether",
      "quantity": "10000000000000000000"
    },
    "beta_ledger": {
      "name": "Ethereum"
    },
    "beta_lock_duration": {
      "type": "seconds",
      "value": 43200
    }
  }
}
@LLFourn

This comment has been minimized.

Copy link
Contributor

LLFourn commented Jan 16, 2019

I really like the way that looks. However, I think it may be sub-optimal in the swap requested/accepted states because these can't be represented as being ledger states.

RFC003 has the terminology of "setup" and "execution". I was thinking that:

{
    "setup" : {
        "current_state" : "requested",
        "alpha_refund_identity": "62E907B15CBF27D5425399EBF6F0FB50EBB88F18",
        "beta_redeem_identity" : "f3f2655c6de2a4c2cc00e58102252e940cc88772",
        "alpha_expiry" :40000,
    }
}

then

{
    "setup" : {
        "current_state" : "accepted",
        "alpha_refund_identity": "62E907B15CBF27D5425399EBF6F0FB50EBB88F18",
        "beta_redeem_identity" : "f3f2655c6de2a4c2cc00e58102252e940cc88772",
        "alpha_redeem_identity" : "292b0736e7e11d8fc2194563472ccd42d56deac4",
        "alpha_expiry" :40000,
        "beta_expiry" : 20000,
        "beta_refund_identity" : "dfd278be493c83828b8845fe292b0736e7e11d8fc2"
    }
}

then

{
    "setup" : {
        "current_state" : "accepted",
        "alpha_refund_identity": "62E907B15CBF27D5425399EBF6F0FB50EBB88F18",
        "alpha_expiry" :40000,
        "beta_redeem_identity" : "f3f2655c6de2a4c2cc00e58102252e940cc88772",
        "alpha_redeem_identity" : "292b0736e7e11d8fc2194563472ccd42d56deac4",
        "beta_expiry" :  20000,
        "beta_refund_identity" : "dfd278be493c83828b8845fe292b0736e7e11d8fc2"
    },
    "execution" : {
        "alpha_ledger" : {
            "current_state" : "funded",
            "funding": {
                "tx": "0x3027661dcac1a70e1ee19a20561b7e304698e52a1eae93fa340e2a1af4afd176"
            }
        },
        "beta_ledger" : {
            "current_state" : "empty"
        }
    }
}

Note this would also remove lock duration stuff out of the swap section and only have rfc002 stuff in there.

@thomaseizinger

This comment has been minimized.

Copy link
Member

thomaseizinger commented Jan 16, 2019

Interesting thought, thanks for mentioning. I'll take that into account. One thing I do would like to avoid is too much nesting. Will see what I can do about it :)

@thomaseizinger

This comment has been minimized.

Copy link
Member

thomaseizinger commented Jan 17, 2019

After some further discussion, @LLFourn and I settled on the following design:

{
    "_links": {
        "redeem": {
            "href": "/swaps/rfc003/ba111c00-672f-4710-9a0c-199abe26b9f3/redeem"
        }
    },
    "role": "Bob",
    "swap": {
        "alpha_asset": {
            "name": "Bitcoin",
            "quantity": "100000000"
        },
        "alpha_ledger": {
            "name": "Bitcoin",
            "network": "regtest"
        },
        "beta_asset": {
            "name": "Ether",
            "quantity": "10000000000000000000"
        },
        "beta_ledger": {
            "name": "Ethereum"
        }
    },
    "state": {
        "outcome": "IN_PROGRESS | SWAPPED | NOT_SWAPPED | INTERNAL_FAILURE",
        "communication": {
            "current_state": "accepted",
            "alpha_refund_identity": "62E907B15CBF27D5425399EBF6F0FB50EBB88F18",
            "alpha_expiry": {
                "type": "blocks",
                "value": 144
            },
            "beta_redeem_identity": "f3f2655c6de2a4c2cc00e58102252e940cc88772",
            "alpha_redeem_identity": "292b0736e7e11d8fc2194563472ccd42d56deac4",
            "beta_expiry": {
                "type": "seconds",
                "value": 43200
            },
            "beta_refund_identity": "dfd278be493c83828b8845fe292b0736e7e11d8fc2"
        },
        "alpha_ledger": {
            "current_state": "redeemed",
            "htlc_location": "37Ju7oTbPfe5qNAHYiAKfdTtVvCSP3TTdL",
            "funding_tx": "0x3027661dcac1a70e1ee19a20561b7e304698e52a1eae93fa340e2a1af4afd176",
            "redeem_tx": "0x52dc163c5fbb1eef1921c21140008860558842c074eb79d3317d194fe0b773fb"
        },
        "beta_ledger": {
            "current_state": "funded",
            "htlc_location": "0xccb882aa9d339b1af3ba01db82cf8ebb443c914c",
            "funding_tx": "0xca7d9d05f474565dcc6f5fef0d8878d6d4e19d2cd8ba9e634e60d6658b879a9c"
        }
    }
}

Notable changes:

  1. The lock-duration move to the communication-state.
  2. There is an overall outcome property that is the single-source of truth for determining the outcome of the swap.
  3. Properties in the individual states accumulate as the states progress, i.e. as the alpha_ledger state progresses from funded to redeemed, the funding_tx is still listed.
  4. At the moment, we don't list an internal failure reason on purpose. Leaking information like that through the API is not really desirable IMO. Internal failures are usually impossible to resolve in an automated fashion. Therefore, you might as well look them up in the log file (for now). As soon as we have the need from an application to show something like that, we can always extend the API.
  5. Due to possible changes to the whole reason thing, I omitted that for now as well.

@comit-network/rust-devs Please voice your opinion!

Edit: I also changes this to a spike ⚗️ because it got slightly out of hand :D

@thomaseizinger thomaseizinger changed the title More (debug) information returned by HTTP API GET swap/:id ⚗️ More (debug) information returned by HTTP API GET swap/:id Jan 17, 2019

@D4nte

This comment has been minimized.

Copy link
Member Author

D4nte commented Jan 17, 2019

Looks Good To Me. Nicely done.

@D4nte D4nte added review and removed work-in-progress labels Jan 20, 2019

@LLFourn

This comment has been minimized.

Copy link
Contributor

LLFourn commented Jan 21, 2019

@thomaseizinger shouldn't the alpha_ledger no longer contain the identities? Otherwise LGTM please create issue to implement this.

@thomaseizinger

This comment has been minimized.

Copy link
Member

thomaseizinger commented Jan 21, 2019

@thomaseizinger shouldn't the alpha_ledger no longer contain the identities? Otherwise LGTM please create issue to implement this.

Because they are negotiated in the communication? If we are talking about the final identities yes. Those in the ledger state would be the transient ones though.

I just saw in the issue body that does should be omitted for now.

Let's remove them then.

@D4nte D4nte modified the milestones: Sprint 6 🔄📖, Sprint 7 Jan 22, 2019

@thomaseizinger

This comment has been minimized.

Copy link
Member

thomaseizinger commented Jan 23, 2019

Closed because spike is done.

@wafflebot wafflebot bot removed the review label Jan 23, 2019

luckysori added a commit that referenced this issue Feb 14, 2019

Rearchitect storage + expand GET /swaps/rfc003/:id
- Return full swap details as per #497 spike results.
- Remove lock duration completely, which should have disappeared in a
  previous PR.
- State store now stores `alice::State`s and `bob::State`s, which
  include more information about the swap and can be more easily
  extended or modified in the future without having to alter the state
  machine that much. Saving the deploy and fund transactions for a
  HTLC is not yet supported.
- Remove seed from HTTP API. The swap seed is now stored inside
  `alice::State` and `bob::State` and it acts as both the secret
  source for Alice, and a Bitcoin redeem and refund address generator
  for both Alice and Bob.
- Emit state machine updates through `mpsc::unbounded()` channel.
  State machine states are sent across threads in a streaming fashion
  from the state machine to the state machine spawner. These are then
  passed on to the storage layer, which translates them and updates
  the corresponding `alice::State` or `bob::State`.
- Deny unknown fields in swap request body.

luckysori added a commit that referenced this issue Feb 14, 2019

Rearchitect storage + expand GET /swaps/rfc003/:id
- Return full swap details as per #497 spike results.
- Remove lock duration completely, which should have disappeared in a
  previous PR.
- State store now stores `alice::State`s and `bob::State`s, which
  include more information about the swap and can be more easily
  extended or modified in the future without having to alter the state
  machine that much. Saving the deploy and fund transactions for a
  HTLC is not yet supported.
- Remove seed from HTTP API. The swap seed is now stored inside
  `alice::State` and `bob::State` and it acts as both the secret
  source for Alice, and a Bitcoin redeem and refund address generator
  for both Alice and Bob.
- Emit state machine updates through `mpsc::unbounded()` channel.
  State machine states are sent across threads in a streaming fashion
  from the state machine to the state machine spawner. These are then
  passed on to the storage layer, which translates them and updates
  the corresponding `alice::State` or `bob::State`.
- Deny unknown fields in swap request body.

luckysori added a commit that referenced this issue Feb 14, 2019

Rearchitect storage + expand GET /swaps/rfc003/:id
- Return full swap details as per #497 spike results.
- Remove lock duration completely, which should have disappeared in a
  previous PR.
- State store now stores `alice::State`s and `bob::State`s, which
  include more information about the swap and can be more easily
  extended or modified in the future without having to alter the state
  machine that much. Saving the deploy and fund transactions for a
  HTLC is not yet supported.
- Remove seed from HTTP API. The swap seed is now stored inside
  `alice::State` and `bob::State` and it acts as both the secret
  source for Alice, and a Bitcoin redeem and refund address generator
  for both Alice and Bob.
- Emit state machine updates through `mpsc::unbounded()` channel.
  State machine states are sent across threads in a streaming fashion
  from the state machine to the state machine spawner. These are then
  passed on to the storage layer, which translates them and updates
  the corresponding `alice::State` or `bob::State`.
- Deny unknown fields in swap request body.

luckysori added a commit that referenced this issue Feb 14, 2019

Rearchitect storage + expand GET /swaps/rfc003/:id
- Return full swap details as per #497 spike results.
- Remove lock duration completely, which should have disappeared in a
  previous PR.
- State store now stores `alice::State`s and `bob::State`s, which
  include more information about the swap and can be more easily
  extended or modified in the future without having to alter the state
  machine that much. Saving the deploy and fund transactions for a
  HTLC is not yet supported.
- Remove seed from HTTP API. The swap seed is now stored inside
  `alice::State` and `bob::State` and it acts as both the secret
  source for Alice, and a Bitcoin redeem and refund address generator
  for both Alice and Bob.
- Emit state machine updates through `mpsc::unbounded()` channel.
  State machine states are sent across threads in a streaming fashion
  from the state machine to the state machine spawner. These are then
  passed on to the storage layer, which translates them and updates
  the corresponding `alice::State` or `bob::State`.
- Deny unknown fields in swap request body.

luckysori added a commit that referenced this issue Feb 14, 2019

Rearchitect storage + expand GET /swaps/rfc003/:id
- Return full swap details as per #497 spike results.
- Remove lock duration completely, which should have disappeared in a
  previous PR.
- State store now stores `alice::State`s and `bob::State`s, which
  include more information about the swap and can be more easily
  extended or modified in the future without having to alter the state
  machine that much. Saving the deploy and fund transactions for a
  HTLC is not yet supported.
- Remove seed from HTTP API. The swap seed is now stored inside
  `alice::State` and `bob::State` and it acts as both the secret
  source for Alice, and a Bitcoin redeem and refund address generator
  for both Alice and Bob.
- Emit state machine updates through `mpsc::unbounded()` channel.
  State machine states are sent across threads in a streaming fashion
  from the state machine to the state machine spawner. These are then
  passed on to the storage layer, which translates them and updates
  the corresponding `alice::State` or `bob::State`.
- Deny unknown fields in swap request body.

luckysori added a commit that referenced this issue Feb 15, 2019

Rearchitect storage + expand GET /swaps/rfc003/:id
- Return full swap details as per #497 spike results.
- Remove lock duration completely, which should have disappeared in a
  previous PR.
- State store now stores `alice::State`s and `bob::State`s, which
  include more information about the swap and can be more easily
  extended or modified in the future without having to alter the state
  machine that much. Saving the deploy and fund transactions for a
  HTLC is not yet supported.
- Remove seed from HTTP API. The swap seed is now stored inside
  `alice::State` and `bob::State` and it acts as both the secret
  source for Alice, and a Bitcoin redeem and refund address generator
  for both Alice and Bob.
- Emit state machine updates through `mpsc::unbounded()` channel.
  State machine states are sent across threads in a streaming fashion
  from the state machine to the state machine spawner. These are then
  passed on to the storage layer, which translates them and updates
  the corresponding `alice::State` or `bob::State`.
- Deny unknown fields in swap request body.
- Reduce e2e tests to only check whether actions are available to
  execute them when appropriate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment