stacks-archive / stacks-transactions-js

The JavaScript library for generating Stacks 2.0 transactions
19 stars 17 forks source link

Add transaction broadcasting #43

Closed yknl closed 4 years ago

yknl commented 4 years ago

Description

This PR adds ability to broadcast transactions to the network.

Usage example:

// using default core node https://core.blockstack.org/v2/broadcast
transaction.broadcast();

// specify a custom core node
transaction.broadcast('https://mycorenode.com');

For details refer to issue #13

Type of Change

Does this introduce a breaking change?

No

Are documentation updates required?

Yes

Testing information

Create and sign a transaction using the transaction builders and call the broadcast() function. This should result in the transaction being broadcasted to the network.

Checklist

hstove commented 4 years ago

FYI - I think you need some slight modifications to the fetch call. For one, the core node won't accept any Content-Type header other than application/octet-stream. Also, I've tried a million different request bodies, and the only thing I got working was passing the Buffer straight to body.

Here's what I have working: https://github.com/blockstack/ux/blob/feat/clarity/packages/rpc-client/src/index.ts#L29

(Note, this is my WIP RPC client, we still need to figure out the story for where this will live eventually.)

yknl commented 4 years ago

Thanks for the info! I pushed some changes to address.

yknl commented 4 years ago

The transaction actually won't be a data-only object. There are state changes such as multi signature signing (not currently implemented) that will mutate it. That said the broadcast functionality can be either separate or contained within the Transaction class. The broadcast operation does not mutate any data. My thinking with the current structure is that it provides a simpler mental model for transactions. Though there may be more complex logic associated with broadcasting in the future where a functional implementation makes more sense.

yknl commented 4 years ago

Agree with the proposal @zone117x

However, my understanding is that the core RPC currently does not provide any useful error responses for us to have this. What I can do now is build in the different error responses but have it return a generic error type for now.

zone117x commented 4 years ago

POST /v2/transaction was recently merged into master. It provides some useful errors, but the structures could definitely be improved. Here's some I recently ran into:

{"reason":"NoSuchContract","error":"transaction rejected","txid":"87ebe3eb2e7921a8b085f4c8bd840eb064ac3189c521d420c2d47d1280fadb6b"}
{"reason":"FeeTooLow(2, 194)","error":"transaction rejected","txid":"cce1ea8f3bf83558e48062280edaf11c166bbd4520760246bbf1667bba1306e6"}
{"reason":"ContractAlreadyExists(QualifiedContractIdentifier { issuer: StandardPrincipalData(26, [191, 142, 130, 98, 60, 56, 12, 216, 112, 147, 29, 72, 181, 37, 213, 225, 42, 77, 103, 130]), name: ContractName(\"hello-world-contract\") })","txid":"4cba360dee56db79e3c7608c88b46adf3b0f5f6b9daa938f0aba183ca7ab475e","error":"transaction rejected"}
yknl commented 4 years ago

Thanks, was not aware of that. I will add the error types in.

diwakergupta commented 4 years ago

POST /v2/transaction was recently merged into master. It provides some useful errors, but the structures could definitely be improved. Here's some I recently ran into:

@zone117x if you have specific ideas / asks on improvements to the errors returned, please do open an issue in the stacks-blockchain repo, thanks!

yknl commented 4 years ago

I updated the PR to pass through the error response from broadcasting.

@zone117x I'm going to address the error code issue in a separate PR since we will need to update the error responses from core first.