feat: add gas estimation to the signer #3017
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughWalkthroughThe update enhances the transaction process by introducing a new gas estimation method and refining the transaction signing process with a new signature method. The Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
rootulp
previously approved these changes
Jan 16, 2024
cmwaters
commented
Jan 17, 2024
evan-forbes
previously approved these changes
Jan 17, 2024
evan-forbes
added
the
backport:v1.x
|
Blocked on celestiaorg/cosmos-sdk#371 |
ninabarbakadze
previously approved these changes
Jan 18, 2024
staheri14
previously approved these changes
Feb 9, 2024
cmwaters
dismissed stale reviews from ninabarbakadze, evan-forbes, staheri14, and rootulp
via
85b38a8
…lestia-app into cal/signer-gas-estimation
evan-forbes
previously approved these changes
Feb 28, 2024
cmwaters
added
the
backport:v1.x
rootulp
previously approved these changes
Feb 28, 2024
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Comment on lines
+246
to
+269
| func (s *Signer) EstimateGas(ctx context.Context, msgs []sdktypes.Msg, opts ...TxOption) (uint64, error) { | ||
| txBuilder := s.txBuilder(opts...) | ||
| if err := txBuilder.SetMsgs(msgs...); err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| if err := s.signTransaction(txBuilder, s.Sequence()); err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| txBytes, err := s.enc.TxEncoder()(txBuilder.GetTx()) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| resp, err := tx.NewServiceClient(s.grpc).Simulate(ctx, &tx.SimulateRequest{ | ||
| TxBytes: txBytes, | ||
| }) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| return resp.GasInfo.GasUsed, nil | ||
| } |
There was a problem hiding this comment.
The EstimateGas method has been added to estimate gas for transactions. This method is crucial for automating gas estimation, aligning with the PR's objective. However, there are a few considerations:
- Ensure that the
Simulatecall accurately reflects the gas estimation for the intended transactions. This might involve verifying that all necessary transaction parameters are set before simulation. - Consider handling potential errors more specifically than just returning them. For instance, if certain errors are expected under normal circumstances, it might be helpful to log them or handle them differently.
- It's good practice to document the expected behavior and limitations of the
EstimateGasmethod, especially since gas estimation can be complex and dependent on various factors.
Comment on lines
+422
to
+435
|
|
||
| func (s *Signer) getSignatureV2(sequence uint64, signature []byte) signing.SignatureV2 { | ||
| sigV2 := signing.SignatureV2{ | ||
| Data: &signing.SingleSignatureData{ | ||
| SignMode: signing.SignMode_SIGN_MODE_DIRECT, | ||
| Signature: signature, | ||
| }, | ||
| Sequence: sequence, | ||
| } | ||
| if sequence == 0 { | ||
| sigV2.PubKey = s.pk | ||
| } | ||
| return sigV2 | ||
| } |
There was a problem hiding this comment.
The getSignatureV2 method constructs a SignatureV2 struct, considering the sequence and signature parameters. Points to note:
- The conditional setting of
PubKeybased on thesequencebeing0is a specific logic that might need clarification in comments. It's mentioned in the PR comments that setting thePubKeyis no longer necessary due to a bug fix in the SDK. Ensure this logic is still required and correctly implemented. - Adding documentation for the method, explaining the significance of the
sequenceparameter and the conditions under which thePubKeyis set, would improve code readability and maintainability.
+ // getSignatureV2 constructs a SignatureV2 struct using the provided sequence and signature.
+ // If the sequence is 0, the signer's public key is also included in the signature.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| func (s *Signer) getSignatureV2(sequence uint64, signature []byte) signing.SignatureV2 { | |
| sigV2 := signing.SignatureV2{ | |
| Data: &signing.SingleSignatureData{ | |
| SignMode: signing.SignMode_SIGN_MODE_DIRECT, | |
| Signature: signature, | |
| }, | |
| Sequence: sequence, | |
| } | |
| if sequence == 0 { | |
| sigV2.PubKey = s.pk | |
| } | |
| return sigV2 | |
| } | |
| // getSignatureV2 constructs a SignatureV2 struct using the provided sequence and signature. | |
| // If the sequence is 0, the signer's public key is also included in the signature. | |
| func (s *Signer) getSignatureV2(sequence uint64, signature []byte) signing.SignatureV2 { | |
| sigV2 := signing.SignatureV2{ | |
| Data: &signing.SingleSignatureData{ | |
| SignMode: signing.SignMode_SIGN_MODE_DIRECT, | |
| Signature: signature, | |
| }, | |
| Sequence: sequence, | |
| } | |
| if sequence == 0 { | |
| sigV2.PubKey = s.pk | |
| } | |
| return sigV2 | |
| } |
Co-authored-by: Rootul P <rootulp@gmail.com>
evan-forbes
approved these changes
Feb 29, 2024
rootulp
approved these changes
Mar 1, 2024
mergify bot
pushed a commit
that referenced
this pull request
Mar 1, 2024
(cherry picked from commit 2b13533) # Conflicts: # pkg/user/signer.go # pkg/user/signer_test.go
rootulp
pushed a commit
that referenced
this pull request
Mar 5, 2024
This is an automatic backport of pull request #3017 done by [Mergify](https://mergify.com). Cherry-pick of 2b13533 has failed: ``` On branch mergify/bp/v1.x/pr-3017 Your branch is up to date with 'origin/v1.x'. You are currently cherry-picking commit 2b13533. (fix conflicts and run "git cherry-pick --continue") (use "git cherry-pick --skip" to skip this patch) (use "git cherry-pick --abort" to cancel the cherry-pick operation) Changes to be committed: modified: pkg/proof/proof_test.go Unmerged paths: (use "git add <file>..." to mark resolution) both modified: pkg/user/signer.go both modified: pkg/user/signer_test.go ``` To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally --- <details> <summary>Mergify commands and options</summary> <br /> More conditions and actions can be found in the [documentation](https://docs.mergify.com/). You can also trigger Mergify actions by commenting on this pull request: - `@Mergifyio refresh` will re-evaluate the rules - `@Mergifyio rebase` will rebase this PR on its base branch - `@Mergifyio update` will merge the base branch into this PR - `@Mergifyio backport <destination>` will backport this PR on `<destination>` branch Additionally, on Mergify [dashboard](https://dashboard.mergify.com) you can: - look at your merge queues - generate the Mergify configuration with the config editor. Finally, you can contact us on https://mergify.com </details> --------- Co-authored-by: Callum Waters <cmwaters19@gmail.com>
ninabarbakadze
pushed a commit
to ninabarbakadze/celestia-app
that referenced
this pull request
Apr 2, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The eventual goal is to have the signer be able to automatically estimate gas