celestia-node icon indicating copy to clipboard operation
celestia-node copied to clipboard

CLI/API: very unclear how to use `state submit-tx`

Open liamsi opened this issue 9 months ago • 16 comments

how is the user supposed to sign the tx in question? even weirder the tx seems to be expected to be hex encoded? even if one used app or some code to generate and sign the tx, it is unclear why then go through the hassle to then submit it via node.

Bare minimum is that:

  • [ ] someone familiar with node / celestia can look at docs and will know exactly how to create a Tx and sign to submit via SubmitTx (all encoding questions resolved + code snippet)

Optional/Nicetohave:

  • [ ] clarify in the docs to the spec so someone who wants to do this e.g. in Rust has all info how to do this or add in another end-point for generating and signing (needs further discussion)

Additionally:

  • [ ] consider removing submitx from the CLI (this does not seem useful as is right now)

liamsi avatar Apr 29 '24 13:04 liamsi

@vgonkivs will take care of removing submit-tx from RPC CLI and I will take care of adding the doc snippet

renaynay avatar Apr 29 '24 13:04 renaynay

@distractedm1nd will take care of updating docs repo with a snippet of how to use SubmitTx endpoint. Thank you

renaynay avatar Apr 30 '24 14:04 renaynay

To use SubmitTx programmatically, you ofc have to import tons of cosmossdk/app stuff, so at that point it doesn’t really make sense to use node at all…. I think it makes the most sense to have the sensible tx types as available endpoints, but maybe remove SubmitTx alltogether, even via the RPC.

I actually think our bases are covered here. We support the following transaction types in node:

  • Transfer
  • SubmitPayForBlob
  • CancelUnbondingDelegation
  • BeginRedelegate
  • Undelegate
  • Delegate
  • GrantFee
  • RevokeGrantFee

So, I would suggest deprecating SubmitTx endpoint and focusing on documenting how to use the transaction specific endpoints. The snippet for SubmitTx will look very similar to this.

Whereas a snippet for a Transfer or SubmitPayForBlob would just be a few lines, way shorter than the above example.

distractedm1nd avatar Apr 30 '24 16:04 distractedm1nd

To use SubmitTx programmatically, you ofc have to import tons of cosmossdk/app stuff, so at that point it doesn’t really make sense to use node at all….

That was my exact reaction on this endpoint as well. @Wondertan and @renaynay kept mentioning that this was mainly a docs issue which might me doubt my intuition on this endpoint a bit. Thanks for confirming. Do you think it might be worth asking 2-3 users about the usefulness of this endpoint before making the final decision to drop it or do you think you really have sufficient data and it does not make sense to double-check this?

liamsi avatar Apr 30 '24 18:04 liamsi

@Bidon15 is asking a few teams and will report back to me, but I am already confident that this endpoint is useless and would already prepare the PR in advance.

distractedm1nd avatar May 02 '24 12:05 distractedm1nd

That was my exact reaction on this endpoint as well. @Wondertan and @renaynay kept mentioning that this was mainly a docs issue which might me doubt my intuition on this endpoint a bit. 

For clarity, all of us three were considering key management outside of the node, as api issues made you doubt the idea of node managing keys in the first place.

After, literally hours, of discussions we agreed that we need both and that different users will tap in to different levels of abstractions depending on the control they want to have. Removing SubmitTx goes against that and limits users only to node managing keys.

Even though I think node managing keys is the simplest UX API you can get and is superior to SubmitTx for obvious reasons. I still think we should not disable it for more sophisticated users. E.g. astria could still use node for submission after struggling with blob.Submit if they had known SubmitTx exist and it had been documented

Wondertan avatar May 02 '24 20:05 Wondertan

It is true that I was convinced that we can keep the key management in node via two arguments:

  • no one requested key management to be outside of node so far and we do not really understand user needs on this sufficiently well (my hope is that @Bidon15 and the devrels team will give us more input on this so we can make an informed decision around this later)
  • the existence for submitTx gives this as an option for power-users and devs anyways (though we don't know if that is what users need)

Why I am agreed with Ryan's assessment nonetheless:

  • even engineers who spent a lot of time with both node and app struggle to quickly write an example in go (let alone in Rust) and have a hard time using submitTx generally; did you even have to use this yourself inside of node? if yes, why not quickly provide a few examples?
  • as mentioned above: we don't really know if anyone would want to use it
  • I don't think the existence of submitTx would change the astria team's mind around the choice to ditch node and go with app for now (but happy to tag @jbowen93 or @joroshiba to confirm/deny)

liamsi avatar May 04 '24 06:05 liamsi

I think if we keep or reintroduce submitTx, then only if the API provided ways to also generate and sign the txs. Apparently that what geth does? cc @adlerjohn @renaynay

liamsi avatar May 04 '24 06:05 liamsi

  • https://geth.ethereum.org/docs/interacting-with-geth/rpc/ns-personal#personalsign
  • https://geth.ethereum.org/docs/tools/clef/apis

liamsi avatar May 04 '24 06:05 liamsi

On one hand:

  • No one uses it
  • It's not a preffered way of tx submission
  • Its complicated to use it
  • Spending time on docs for a niche use case is suboptimal, while there are other critical gaps to fil

On the other hand:

  • Astria requesting control over retries is a use case for the SubmitTx, so we know there is someone who may want that.
  • Its not clear how disabling this endpoint helps anyhow. One would argue it restricts the API.
  • Node providing TX generation and signing capabilities over API is a great addition to SubmitTx ux, but not a requirement. SubmitTx can be kept until these two are provided.

Wondertan avatar May 04 '24 09:05 Wondertan

Could you write the documentation for how to use it in go and Rust (latter particularly relevant for the Astria and retry argument)? Otherwise, I'm in favour of removing it as it does not seem practical and is a purely theoretically useful endpoint.

liamsi avatar May 05 '24 10:05 liamsi

I can write it for Go, but for Rust it would be slow and not the best use of my time(considering current priorities). If you think Rust docs are a must to keep SubmitTx, then lets remove it.

Wondertan avatar May 06 '24 10:05 Wondertan

I can write it for Go

If we decide to keep it that would be very appreciated.

For the API oevrhaul (aka canonical API / v1 API), I would consider this kind of sign flow: https://github.com/ethereum/go-ethereum/blob/master/cmd/clef/sign_flow.png

In that case, there would also be a submitTx endpoint but it would be very clear how to use it.

liamsi avatar May 07 '24 14:05 liamsi

cc: @evan-forbes

Bidon15 avatar May 07 '24 15:05 Bidon15

Apparently that what geth does?

The docs linked above look old. I think personal isn't used anymore. Regardless, the RPC endpoint is eth_signtransaction, which requires the user to provide a JSON of the transaction fields: https://ethereum.org/en/developers/docs/apis/json-rpc/#eth_signtransaction

adlerjohn avatar May 13 '24 00:05 adlerjohn

Yeah, the functionality was moved to Clef (also linked above).

liamsi avatar May 13 '24 11:05 liamsi