x/payment/pfd.go: Add canonical PFD constructor/signer/submittor methods
#352
Conversation
There was a problem hiding this comment.
I think we should also create an issue to see if we can also reduce the exposed API after introducing these.
Only one suggested change, wish we would have thought to move this to the app after the dalc or even sooner. Really great work @renaynay . I think this will save countless headaches and foot guns.
x/payment/pfd.go
Outdated
| // SignPayForData signs a PayForData transaction. | ||
| func SignPayForData( | ||
| signer *types.KeyringSigner, | ||
| pfd *types.MsgWirePayForData, | ||
| gasLimOption types.TxBuilderOption, |
There was a problem hiding this comment.
instead of doing only a gasLim option, do you think it makes sense to do ...types.TxBuilderOption?
There was a problem hiding this comment.
@evan-forbes does the gas lim get set by default if no TxBuilderOption is included for it?
There was a problem hiding this comment.
not 100% sure, but I don't think so
There was a problem hiding this comment.
Okay can you check this commit to see how i've implemented it: 931fbd5
x/payment/pfd.go
Outdated
| var ( | ||
| // shareSizes includes all the possible share sizes of the given data | ||
| // that the signer must sign over. | ||
| shareSizes = []uint64{16, 32, 64, 128} | ||
| ) |
x/payment/pfd.go
Outdated
| // SubmitPayForData constructs, signs and synchronously submits a PayForData | ||
| // transaction, returning a sdk.TxResponse upon submission. | ||
| func SubmitPayForData( | ||
| ctx context.Context, | ||
| signer *types.KeyringSigner, | ||
| conn *grpc.ClientConn, | ||
| nID namespace.ID, | ||
| data []byte, | ||
| gasLim uint64, | ||
| ) (*sdk.TxResponse, error) { | ||
| pfd, err := BuildPayForData(ctx, signer, conn, nID, data, gasLim) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| signed, err := SignPayForData(signer, pfd, types.SetGasLimit(gasLim)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| rawTx, err := signer.EncodeTx(signed) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| txResp, err := types.BroadcastTx(ctx, conn, sdk_tx.BroadcastMode_BROADCAST_MODE_BLOCK, rawTx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return txResp.TxResponse, nil | ||
| } |
There was a problem hiding this comment.
finally, the prophesied good api for submitting a payForData has come 🙌
|
oh shoot, one last thing minor nit pick, I would prefer to name the file something else instead of the shorthand version, just to be consistent. |
…ame pfd.go to payfordata.go
…thods (celestiaorg#352) * feat: add canonical PFD constructor/signer/submittor methods to payment module * refactor: add types.TxBuilderOpts as param for SubmitPayForData | rename pfd.go to payfordata.go
This PR adds 3 methods:
BuildPayForDataSignPayForDataSubmitPayForDataCelestia-node (and other repos depending on this message type) should import these methods from app.
Once this PR is merged, I will close celestiaorg/celestia-node#619 and reopen a PR that updates the celestia-app dep to the latest celestia-app release and uses these methods under the hood.