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

test: increase e2e stability #419

Merged
merged 17 commits into from
Mar 20, 2025
Merged

Conversation

gjermundgaraba
Copy link
Contributor

@gjermundgaraba gjermundgaraba commented Mar 17, 2025

Description

closes: #XXXX


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Linked to GitHub issue with discussion and accepted design, OR link to spec that describes this work.
  • Wrote unit and integration tests.
  • Added relevant natspec and godoc comments.
  • Provide a conventional commit message to follow the repository standards.
  • Re-reviewed Files changed in the GitHub PR explorer.
  • Review SonarCloud Report in the comment section below once CI passes.

Copy link

codecov bot commented Mar 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.86%. Comparing base (e981ff2) to head (909ab1b).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #419   +/-   ##
=======================================
  Coverage   99.86%   99.86%           
=======================================
  Files          15       15           
  Lines         744      744           
=======================================
  Hits          743      743           
  Misses          1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gjermundgaraba gjermundgaraba changed the title test: increase e2e stability Do not merge: test: increase e2e stability Mar 18, 2025
@gjermundgaraba gjermundgaraba marked this pull request as ready for review March 18, 2025 15:20
@gjermundgaraba gjermundgaraba requested a review from srdtrk as a code owner March 18, 2025 15:20
@gjermundgaraba gjermundgaraba marked this pull request as draft March 18, 2025 16:36
@gjermundgaraba gjermundgaraba changed the title Do not merge: test: increase e2e stability test: increase e2e stability Mar 18, 2025
@gjermundgaraba gjermundgaraba marked this pull request as ready for review March 20, 2025 09:22
@gjermundgaraba
Copy link
Contributor Author

Looks like things are much more stable now: https://github.com/cosmos/solidity-ibc-eureka/actions/runs/13965649544

Copy link
Member

@srdtrk srdtrk left a comment

Choose a reason for hiding this comment

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

Lgtm. Left some questions

Comment on lines +121 to +151
func retryConfig() grpc.DialOption {
policy := `{
"methodConfig": [{
"name": [{}],
"retryPolicy": {
"MaxAttempts": 4,
"InitialBackoff": ".01s",
"MaxBackoff": ".01s",
"BackoffMultiplier": 1.0,
"RetryableStatusCodes": [
"CANCELLED",
"UNKNOWN",
"DEADLINE_EXCEEDED",
"NOT_FOUND",
"ALREADY_EXISTS",
"PERMISSION_DENIED",
"RESOURCE_EXHAUSTED",
"FAILED_PRECONDITION",
"ABORTED",
"OUT_OF_RANGE",
"UNIMPLEMENTED",
"INTERNAL",
"UNAVAILABLE",
"DATA_LOSS",
"UNAUTHENTICATED"
]
}
}]
}`

return grpc.WithDefaultServiceConfig(policy)
Copy link
Member

Choose a reason for hiding this comment

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

Is it not possible to create this in a type safe way or with a builder. (Not that I think we should build one, but asking if grpc already has a builder or something similar)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, good question. I'll check!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't seem like this exists in the go version of the library :/

@@ -102,7 +102,7 @@ func (e *Ethereum) BroadcastTx(ctx context.Context, userKey *ecdsa.PrivateKey, g

func (e Ethereum) ForgeScript(deployer *ecdsa.PrivateKey, solidityContract string, args ...string) ([]byte, error) {
args = append(args, "script", "--rpc-url", e.RPC, "--private-key",
hex.EncodeToString(deployer.D.Bytes()), "--broadcast",
hex.EncodeToString(crypto.FromECDSA(deployer)), "--broadcast",
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, curious if you know why it was less stable before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not 100%, but there is a different handling of the private key bytes (something about fixed sized buffers and whatnot). But this seems to be the correct way, at least.

@gjermundgaraba gjermundgaraba merged commit 6624fa6 into main Mar 20, 2025
112 checks passed
@gjermundgaraba gjermundgaraba deleted the gjermund/e2e-stability-work branch March 20, 2025 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants