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

Enable dataposter for validator contract wallet #1788

Merged
merged 27 commits into from
Aug 15, 2023

Conversation

anodar
Copy link
Contributor

@anodar anodar commented Jul 31, 2023

No description provided.

@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Jul 31, 2023
@anodar anodar marked this pull request as ready for review August 3, 2023 19:04
@anodar anodar requested a review from PlasmaPower August 3, 2023 19:04
@PlasmaPower PlasmaPower requested review from amsanghi and removed request for PlasmaPower August 9, 2023 19:29
amsanghi
amsanghi previously approved these changes Aug 10, 2023
Copy link
Contributor

@amsanghi amsanghi left a comment

Choose a reason for hiding this comment

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

LGTM!

Just one concern.

@@ -68,8 +68,8 @@ func (s *Storage) Put(_ context.Context, index uint64, prev, new *storage.Queued
s.queue = append(s.queue, new)
} else if index >= s.firstNonce {
queueIdx := int(index - s.firstNonce)
if queueIdx > len(s.queue) {
return fmt.Errorf("attempted to set out-of-bounds index %v in queue starting at %v of length %v", index, s.firstNonce, len(s.queue))
Copy link
Contributor

Choose a reason for hiding this comment

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

any particular reason for suppressing this error? earlier out-of-bound issues will be masked as notEqual error now.

Also this might hide one off errors in s.queue. Please confirm with Lee once.

Copy link
Contributor Author

@anodar anodar Aug 15, 2023

Choose a reason for hiding this comment

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

Reverted this and addressed the part that was requiring out-of-bound indexes so that it's no longer needed.

@anodar anodar deleted the branch master August 15, 2023 11:57
@anodar anodar closed this Aug 15, 2023
@anodar anodar reopened this Aug 15, 2023
@anodar anodar changed the base branch from dataposter-for-validator to master August 15, 2023 11:58
@anodar anodar dismissed amsanghi’s stale review August 15, 2023 11:58

The base branch was changed.

@anodar anodar requested a review from amsanghi August 15, 2023 14:05
@anodar anodar enabled auto-merge August 15, 2023 14:20
Data: data,
GasFeeCap: gasFeeCap,
GasTipCap: gasTipCap,
GasPrice: gp,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Specifying GasPrice is redundant if you already specify GasFeeCap and GasTipCap

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -326,6 +403,7 @@ func (v *ContractValidatorWallet) TestTransactions(ctx context.Context, txs []*t
Value: totalAmount,
Data: realData,
}
log.Error("anodar testTransactions pendingcallcontract", "msg", msg)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this was left in by mistake

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch!

Copy link
Collaborator

@PlasmaPower PlasmaPower left a comment

Choose a reason for hiding this comment

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

LGTM

@anodar anodar merged commit 9aa1016 into master Aug 15, 2023
8 checks passed
@anodar anodar deleted the dataposter-for-validator-contract-wallet branch August 15, 2023 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants