L1 Data submission config and validium blob submitter #1788
Conversation
gauravahuja
requested a deployment
to
docker-build-and-e2e
GitHub Actions
Waiting
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| companion object { | ||
| fun valueOfIgnoreCase(name: String): DataSubmission { | ||
| return DataSubmission.entries.firstOrNull { it.name.equals(name, ignoreCase = true) } |
There was a problem hiding this comment.
Bug: Unused property in DataSubmission enum
The DataSubmission enum defines a mame property but valueOfIgnoreCase uses it.name instead of it.mame for comparison. This is inconsistent with the SignerType enum pattern which uses it.mame. The defined property serves no purpose and creates dead code, breaking the established pattern in the codebase.
There was a problem hiding this comment.
please address this. Also, I don't thing we need the name property...
There was a problem hiding this comment.
ok, I was just following what was done for a similar TOML config enum SignerType
| // eth_estimateGas would fail because we submit multiple blob tx | ||
| // and 2nd would fail with revert reason | ||
| useEthEstimateGas = false, | ||
| val transactionManager = createTransactionManager( |
There was a problem hiding this comment.
Bug: Null pointer exception for Validium blob gas cap
The code force-unwraps maxFeePerBlobGasCap!! when creating gas providers for both ROLLUP and VALIDIUM modes. Since maxFeePerBlobGasCap is nullable and Validium doesn't use EIP-4844 blobs, this will throw a NullPointerException when running in Validium mode if the configuration doesn't provide this optional value. The gas provider creation should handle Validium differently or require the value only for ROLLUP mode.
Additional Locations (1)
fluentcrafter
left a comment
fluentcrafter
left a comment
There was a problem hiding this comment.
I haven't finished the review. Leaving some comments in the meantime
| } | ||
| } | ||
|
|
||
| class TomlDataSubmissionDecoder : Decoder<L1SubmissionConfigToml.DataSubmission> { |
There was a problem hiding this comment.
why do we need a specific decoder. Toml library shall handle that out for the box, no?
There was a problem hiding this comment.
Again, i did not experiment with enum and toml library, I just followed a similar pattern we have for SignerType. You can notice the TomlSignerTypeDecoder just above this class.
|
|
||
| companion object { | ||
| fun valueOfIgnoreCase(name: String): DataSubmission { | ||
| return DataSubmission.entries.firstOrNull { it.name.equals(name, ignoreCase = true) } |
There was a problem hiding this comment.
please address this. Also, I don't thing we need the name property...
| val dataSubmission: DataSubmission = DataSubmission.ROLLUP, | ||
| ) { | ||
|
|
||
| enum class DataSubmission(val mame: String) { |
There was a problem hiding this comment.
No, name is an enum class attribute, I can either override name or have a different variable name. I just followed a similar enum that we use for TOML config SignerType
| web3jClient: Web3j, | ||
| smartContractErrors: SmartContractErrors, | ||
| useEthEstimateGas: Boolean, | ||
| ): LineaValidiumSmartContractClient { |
There was a problem hiding this comment.
this looks duplicated. Have you considered
fun createLineaContractClient(
dataAvailabiltyType: DataSubmission
contractAddress: String,
transactionManager: AsyncFriendlyTransactionManager,
contractGasProvider: ContractGasProvider,
web3jClient: Web3j,
smartContractErrors: SmartContractErrors,
useEthEstimateGas: Boolean,
):
There was a problem hiding this comment.
Yes, that is better.
| val aggregation: AggregationSubmissionConfig, | ||
| val dataSubmission: DataSubmission, | ||
| ) : FeatureToggle { | ||
| enum class DataSubmission { |
There was a problem hiding this comment.
DataSubmission is not appropriate IMO
suggestions:
- DataAvailability
- DataStorageMode
- L2DaType
@julien-marchand any inputs on this?
This will have impact on logic/proving once filly implemented
| val toml = """ | ||
| [l1-submission] | ||
| disabled = true | ||
| data-submission = "validium" |
There was a problem hiding this comment.
In our meeting with @julien-marchand we agreed on this config layout
[l1-submission.blob]
data-availability = "ROLLUP, PRIVIDIUM, VALIDIUM"
What's the motivation/rationale to:
- put it on upper level,
l1-submission - change the name to data-submission?
There was a problem hiding this comment.
- I did not put under
l1-submission.blobbecause validium also involves shnarf submission. So overall it is how data (shnarf and blobs) are available not just blobs. - I forgot what name we had decided on, I will update it to
data-availability.
gauravahuja
force-pushed
the
validium_submitter
branch
from
845c930 to
2763531
Compare
This PR implements issue(s) #
Checklist
Note
Adds a Validium L1 data submission path selectable via config, introduces a Validium smart contract client, and generalizes blob submission to work with either Rollup or Validium; also migrates usages to LineaRollupContractVersion.
dataSubmission(ROLLUP | VALIDIUM) toL1SubmissionConfigand TOML (L1SubmissionConfigToml), with decoderTomlDataSubmissionDecoderand parsing tests updated.l1-submission.data-submissionand addcreateLineaValidiumContractClient.BlobSubmissionCoordinatortoLineaSmartContractClient; addValidiumBlobSubmitter; updateL1ShnarfBasedAlreadySubmittedBlobsFilterto acceptLineaSmartContractClientReadOnly.Web3JLineaValidiumSmartContractClientandLineaValidiumEnhancedWrapper.LineaRollupContractVersionand update builders (Web3JLineaRollupFunctionBuilders).LineaRollupContractVersion; wire changes in submission/finalization and state recovery tests; minor assertion tweak in config parsing test.Written by Cursor Bugbot for commit 3fe801a. This will update automatically on new commits. Configure here.