feat!: using celestia RPC instead of Rest API #544
Conversation
…-change-to-use-rpc-client-as-api-gateway-is-gonna-get-deprecated-for-writing-to-celestia
| } | ||
| blobs := []*blob.Blob{blockBlob} | ||
|
|
||
| estimatedGas := DefaultEstimateGas(uint32(len(data))) |
There was a problem hiding this comment.
shouldn't we estimate here the len(blobs) vs len(data)?
There was a problem hiding this comment.
no, it's the same as in celestia-node
| if txResponse.Code != 0 { | ||
| c.logger.Error("Failed to submit DA batch. Emitting health event and trying again", "txResponse", txResponse.RawLog, "code", txResponse.Code) | ||
| res, err := da.SubmitBatchHealthEventHelper(c.pubsubServer, c.ctx, false, errors.New(txResponse.RawLog)) | ||
| if txResponse == nil { |
There was a problem hiding this comment.
Seems like we can skip this response and the next (txresponse.Code) as they are doing it currently in their new api: https://github.com/celestiaorg/celestia-node/blob/ccf9b56317ae8941f91c5d09f24fe8422bbe64bb/state/core_access.go#L256
There was a problem hiding this comment.
validating txResponse != nil is good practice to avoid possible panics.
i'll remove the code check
| daHeight := uint64(txResponse.Height) | ||
| if daHeight == 0 { | ||
| c.logger.Debug("Failed to receive DA batch inclusion result. Waiting for inclusion", "txHash", txResponse.TxHash) | ||
| daHeight, err = c.waitForTXInclusion(txResponse.TxHash) |
There was a problem hiding this comment.
Why did we remove the inclusion test?
the submitPayForBlock broadcasts the tx in sync mode which means it's not necessarily gonna get included in a block.
Also from their docs: https://docs.celestia.org/nodes/transaction-resubmission#transaction-resubmission
There was a problem hiding this comment.
it's actually in block mode and by their docs it will fail after 75 sec, and the caller expected to retry.
this implementation is not relevant IMO
We do plan to get the inclusion proof/commitment, we have separate issue for it.
…o-use-rpc-client-as-api-gateway-is-gonna-get-deprecated-for-writing-to-celestia
da/celestia/celestia.go
Outdated
| return res | ||
| err := errors.New("txResponse is nil") | ||
| c.logger.Error("Failed to submit DA batch", "error", err) | ||
| return da.ResultSubmitBatch{ |
There was a problem hiding this comment.
why are we returning here and not retrying and submitting an health event? the node will simply panic.
There was a problem hiding this comment.
well it's not supposed to happen, so IMO panic is the right choice (similar to invariant break)
WDYT
There was a problem hiding this comment.
I don't see it as an invariant broken or a bug so I don't think we should panic, especially if retry could help (which is why we added it in the previous version as it was the celestia's network instability/response).
Also if we panic and restart don't see why would it change anything if it depends on the celestia network.
There was a problem hiding this comment.
it's not depend on the network
it's part of the API contract. u don't suppose to get nil resp and nil err
but ok I'll change it
|
tests failing |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #544 +/- ##
==========================================
- Coverage 52.43% 48.17% -4.27%
==========================================
Files 100 98 -2
Lines 14764 14484 -280
==========================================
- Hits 7742 6978 -764
- Misses 5911 6535 +624
+ Partials 1111 971 -140 ☔ View full report in Codecov by Sentry. |
mtsitrin
deleted the
mtsitrin/398-change-to-use-rpc-client-as-api-gateway-is-gonna-get-deprecated-for-writing-to-celestia
branch
PR Standards
Opening a pull request should be able to meet the following requirements
For Author:
godoccommentsFor Reviewer:
After reviewer approval: