Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(taiko-client): soft block driver APIs #18273

Open
wants to merge 70 commits into
base: main
Choose a base branch
from

Conversation

davidtaikocha
Copy link
Member

@davidtaikocha davidtaikocha commented Oct 21, 2024

Based on the latest design.

API preview: https://taikoxyz.github.io/taiko-mono/

taiko-geth changes: taikoxyz/taiko-geth#342

@davidtaikocha
Copy link
Member Author

#18273 (comment)

added

@dantaik dantaik changed the title feat(taiko-client): preconfirmation driver APIs feat(taiko-client): soft block driver APIs Oct 23, 2024
@davidtaikocha davidtaikocha marked this pull request as ready for review November 4, 2024 15:39
}

var txListBytes []byte
if s.rpc.L2.ChainID.Cmp(params.HeklaNetworkID) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we have this similiar code elsewhere it probably makes sense to just have a txListDecompressor.TryDecompress() that takes a chainID, and does the comparison inside that function. I dont think we need two exported functions. They take the same parameters.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened a PR here: #18444

new(big.Int).SetUint64(reqBody.TransactionBatch.BlockID),
)
if err != nil && err.Error() != ethereum.NotFound.Error() {
return err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return err should be

return c.JSON(http.StatusBadRequest, map[string]string{
				"error": err.Error(),
			})

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed

return c.JSON(http.StatusBadRequest, map[string]string{"error": "soft block has already been marked as ended"})
}
if l1Origin.EndOfPreconf {
return c.JSON(http.StatusBadRequest, map[string]string{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe helper function for cleaner code, returnError(statusCode int, error string) so we can just do

return c.JSON(statusCode, map[string]string{error} once, i hate the look of all these repetetive inline maps. And then if we want we can add logging/etc within that function. idk.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a returnError helper method: 479711d

// @param either `end_of_block`, `end_of_preconf` or empty
BatchMarker TransactionBatchMarker `json:"batchType"`
// @param signature string Signature of this transaction batch
Signature string `json:"signature"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Signature string `json:"signature"`
Signature string `json:"signature" rlp:"-"`

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silence rlp encode and decode, so we don't need to copy the heavy struct at line59.

Copy link
Member Author

@davidtaikocha davidtaikocha Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the same as passing an empty string here? Since the test here:

payload, err := rlp.EncodeToBytes(txBatch)
s.Nil(err)
s.NotEmpty(payload)
sig, err := crypto.Sign(crypto.Keccak256(payload), preconferPrivKey)
s.Nil(err)
is passed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants