Skip to content

Conversation

@masihyeganeh
Copy link
Contributor

@masihyeganeh masihyeganeh commented May 6, 2025

This change is Reviewable

@masihyeganeh masihyeganeh requested a review from a team as a code owner May 6, 2025 09:14
@masihyeganeh masihyeganeh requested review from TxCorpi0x, miladz68 and ysv and removed request for a team May 6, 2025 09:14
miladz68
miladz68 previously approved these changes May 6, 2025
Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @TxCorpi0x and @ysv)

ysv
ysv previously approved these changes May 6, 2025
Copy link
Collaborator

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @TxCorpi0x)

TxCorpi0x
TxCorpi0x previously approved these changes May 8, 2025
Copy link
Contributor

@TxCorpi0x TxCorpi0x left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @masihyeganeh)

@masihyeganeh masihyeganeh dismissed stale reviews from TxCorpi0x, ysv, and miladz68 via 73449f6 May 9, 2025 11:43
Copy link
Collaborator

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @masihyeganeh)


.github/workflows/release.yml line 10 at r2 (raw file):

jobs:
  release:
    runs-on: ubuntu-24.04

I'm not sure what we should do for faucet.
maybe running on github-hosted runner ins still an option for faucet

If no then we should probably register our runners to be organization wide (not per repo as it is now)

maybe makes sense to check if it works if we run on ubuntu ?

miladz68
miladz68 previously approved these changes May 12, 2025
Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @masihyeganeh)

Copy link
Contributor Author

@masihyeganeh masihyeganeh left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ysv)


.github/workflows/release.yml line 10 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

I'm not sure what we should do for faucet.
maybe running on github-hosted runner ins still an option for faucet

If no then we should probably register our runners to be organization wide (not per repo as it is now)

maybe makes sense to check if it works if we run on ubuntu ?

I guess GitHub-hosted runners can run the CI for faucet. But I need this PR to merge, so I can test it

ysv
ysv previously approved these changes May 13, 2025
Copy link
Collaborator

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @masihyeganeh)

@masihyeganeh masihyeganeh dismissed stale reviews from ysv and miladz68 via 9af6f6f May 13, 2025 10:54
Copy link
Contributor Author

@masihyeganeh masihyeganeh left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @masihyeganeh)

Copy link
Contributor Author

@masihyeganeh masihyeganeh left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @masihyeganeh)

ysv
ysv previously approved these changes May 13, 2025
Copy link
Collaborator

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @masihyeganeh)

Copy link
Contributor Author

@masihyeganeh masihyeganeh left a comment

Choose a reason for hiding this comment

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

Reviewed 16 of 16 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @masihyeganeh)

Copy link
Contributor Author

@masihyeganeh masihyeganeh left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @masihyeganeh)

Copy link
Contributor Author

@masihyeganeh masihyeganeh left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @masihyeganeh)

Copy link
Collaborator

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed 16 of 16 files at r4, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @masihyeganeh)


client/coreum/batcher.go line 125 at r6 (raw file):

			_ = parallel.Run(ctx, func(ctx context.Context, spawn parallel.SpawnFn) error {
				for _, fundingAddress := range b.fundingAddresses {
					fundingAddress := fundingAddress

I'm not sure we should change this here
This was probably done on purpose

Copy link
Contributor

@TxCorpi0x TxCorpi0x left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @masihyeganeh)

Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, 16 of 16 files at r4, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @masihyeganeh)

Copy link
Contributor Author

@masihyeganeh masihyeganeh left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ysv)


client/coreum/batcher.go line 125 at r6 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

I'm not sure we should change this here
This was probably done on purpose

It was needed in older versions of Go. Not anymore.
Let me know if you think it is still needed.

Copy link
Collaborator

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @masihyeganeh)


client/coreum/batcher.go line 125 at r6 (raw file):

Previously, masihyeganeh (Masih Yeganeh) wrote…

It was needed in older versions of Go. Not anymore.
Let me know if you think it is still needed.

thanks, it is clear now

@masihyeganeh masihyeganeh merged commit f11c210 into master May 19, 2025
4 checks passed
@masihyeganeh masihyeganeh deleted the masih/fix-grpc-client branch May 19, 2025 09:58
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.

5 participants