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: add the ability to specifiy the txsim master account #2292

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

evan-forbes
Copy link
Member

@evan-forbes evan-forbes commented Aug 17, 2023

Overview

This PR adds the ability to specify a specific account to use as master for txsim. This is useful for when we don't want to always use the account with the most funds in our wallet.

closes #2286
kinda sorta blocking #2197

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@evan-forbes evan-forbes self-assigned this Aug 17, 2023
@MSevey MSevey requested a review from a team August 17, 2023 00:28
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

LGTM. We might be getting to the stage where we want to start using the options paradigm (or even having a config). masterAccName, seed and pollTime all have defaults and don't need to be set. Arguably grpcEndpoints and rpcEndpoints also have default values. This may make the constructor a bit more tidy and allow us to continue adding options in the future

@evan-forbes
Copy link
Member Author

We might be getting to the stage where we want to start using the options paradigm (or even having a config).

yeah I was thinking the same thing. a config would be more readable to me than a bash script with 10 flags lol

@evan-forbes evan-forbes merged commit 2102189 into main Aug 17, 2023
@evan-forbes evan-forbes deleted the evan/specify-master-account branch August 17, 2023 13:44
@evan-forbes evan-forbes added the backport:v1.x PR will be backported automatically to the v1.x branch upon merging label Aug 17, 2023
mergify bot pushed a commit that referenced this pull request Aug 17, 2023
## Overview

This PR adds the ability to specify a specific account to use as master
for txsim. This is useful for when we don't want to always use the
account with the most funds in our wallet.

closes #2286
kinda sorta blocking #2197

## Checklist

- [x] New and updated code has appropriate documentation
- [x] New and updated code has new and/or updated testing
- [x] Required CI checks are passing
- [x] Visual proof for any user facing features like CLI or
documentation updates
- [x] Linked issues closed with keywords

(cherry picked from commit 2102189)

# Conflicts:
#	test/e2e/simple_test.go
@cmwaters
Copy link
Contributor

cmwaters commented Aug 17, 2023

We might be getting to the stage where we want to start using the options paradigm (or even having a config).

yeah I was thinking the same thing. a config would be more readable to me than a bash script with 10 flags lol

I think the devops folks will want flags all the way :)

evan-forbes added a commit that referenced this pull request Aug 17, 2023
…2292) (#2299)

This is an automatic backport of pull request #2292 done by
[Mergify](https://mergify.com).
Cherry-pick of 2102189 has failed:
```
On branch mergify/bp/v1.x/pr-2292
Your branch is up to date with 'origin/v1.x'.

You are currently cherry-picking commit 2102189.
  (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:   app/test/square_size_test.go
	modified:   test/cmd/txsim/cli.go
	modified:   test/txsim/account.go
	modified:   test/txsim/run.go
	modified:   test/txsim/run_test.go

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)
	deleted by us:   test/e2e/simple_test.go

```


To fix up this pull request, you can check it out locally. See
documentation:
https://docs.github.com/en/github/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: Evan Forbes <[email protected]>
Co-authored-by: evan-forbes <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:v1.x PR will be backported automatically to the v1.x branch upon merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the ability to specify the master account in txsim
4 participants