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

add --nonce override flag to kwil-cli database commands #337

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

jchappelow
Copy link
Member

@jchappelow jchappelow commented Oct 4, 2023

kwil-cli,pkg/client: add --nonce override flag to database cmds
This adds the ability to set a nonce when making a transaction.

Rather than giving every method a nonce input arg, I've added a
WithNonceFunc option to pkg/client.Client. This is perhaps a
little strange but the cascade from a new argument was rather
large, and it seemed a little out of place as the other arguments
pertain to the action rather than the transaction containing it.
Can be switched if we'd prefer.

See my review for notes about the pkg/client change. Very happy to change it.

@jchappelow jchappelow changed the title add --nonce override flag to kwil-cli database add --nonce override flag to kwil-cli database commands Oct 4, 2023
@jchappelow jchappelow force-pushed the cli-nonce-override branch 2 times, most recently from 8a57e18 to f1dea6d Compare October 5, 2023 14:13
@@ -18,7 +18,7 @@ const (

type RoundTripper func(ctx context.Context, client *client.Client, conf *config.KwilCliConfig) error

func DialClient(ctx context.Context, flags uint8, fn RoundTripper) error {
func DialClient(ctx context.Context, flags uint8, nonce int64, fn RoundTripper) error {
Copy link
Member Author

Choose a reason for hiding this comment

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

DialClient is the one-shot client request helper. If the RoundTripper func wants to make multiple transactions this would have to change (or let it be auto).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm this feels wrong to me.

I think a more logical approach would be to add some sort of option to the commands that might require a nonce (e.g., client.ExecuteAction). This seems more logical, as it doesn't require the pkg/client consumer to reinstantiate a client for each transaction if they want fine-grained control over their nonces. Also, not every method on Client uses nonces; if somebody is simply reading data, it seems weird to concern them with a nonce

Copy link
Member Author

Choose a reason for hiding this comment

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

I think a more logical approach would be to add some sort of option to the commands that might require a nonce (e.g., client.ExecuteAction).

Yup, I actually completely agree with that. Had started that way but the already-variadic ExecuteAction actually sent me in a direction with no method signature changes. It out to be not bad. Changes made.

This seems more logical, as it doesn't require the pkg/client consumer to reinstantiate a client for each transaction if they want fine-grained control over their nonces

Definitely switching this up, but it was really up to the nonceFunc implementation, which can be defined to return different value each time (imagine a simple counter, or the it's a closure bound to a variable that the consumer mutates between requests, or more complex logic like doing some other RPCs before returning a value, etc.). Not needed and confusing though. Modifying the method signatures instead.

pkg/client/tx.go Outdated Show resolved Hide resolved
@jchappelow jchappelow marked this pull request as ready for review October 5, 2023 14:21
@jchappelow jchappelow force-pushed the cli-nonce-override branch 2 times, most recently from 61113ba to da75a9e Compare October 5, 2023 14:41
Copy link
Contributor

@Yaiba Yaiba left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/client/tx.go Outdated Show resolved Hide resolved
@jchappelow
Copy link
Member Author

@charithabandi Was this working OK for you? I thought you said you used the --nonce flag to test this morning but wasn't sure. Shall we get this one in before our two abci PRs?

@@ -18,7 +18,7 @@ const (

type RoundTripper func(ctx context.Context, client *client.Client, conf *config.KwilCliConfig) error

func DialClient(ctx context.Context, flags uint8, fn RoundTripper) error {
func DialClient(ctx context.Context, flags uint8, nonce int64, fn RoundTripper) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm this feels wrong to me.

I think a more logical approach would be to add some sort of option to the commands that might require a nonce (e.g., client.ExecuteAction). This seems more logical, as it doesn't require the pkg/client consumer to reinstantiate a client for each transaction if they want fine-grained control over their nonces. Also, not every method on Client uses nonces; if somebody is simply reading data, it seems weird to concern them with a nonce

cmd/kwil-cli/cmds/common/roundtripper.go Outdated Show resolved Hide resolved
pkg/client/tx.go Outdated Show resolved Hide resolved
@jchappelow
Copy link
Member Author

Went to the more sensible approach. We don't need a wacky way to patch in programmable nonce logic like that. Will retest later tonight to make sure it still works though.

This adds the ability to set a nonce when making a transaction.

Rather than giving every method a nonce input arg, I've added a
WithNonceFunc option to pkg/client.Client. This is perhaps a
little strange but the cascade from a new argument was rather
large, and it seemed a little out of place as the other arguments
pertain to the action rather than the transaction containing it.
Can be switched if we'd prefer.
@charithabandi
Copy link
Contributor

@charithabandi Was this working OK for you? I thought you said you used the --nonce flag to test this morning but wasn't sure. Shall we get this one in before our two abci PRs?

Yup, tested it out again, it works for me.

@brennanjl brennanjl merged commit f9c5409 into main Oct 6, 2023
2 checks passed
@brennanjl brennanjl deleted the cli-nonce-override branch October 6, 2023 15:23
@jchappelow jchappelow added this to the v0.6.0 milestone Nov 6, 2023
@Yaiba Yaiba mentioned this pull request Dec 12, 2023
2 tasks
brennanjl pushed a commit that referenced this pull request Feb 26, 2024
This adds the ability to set a nonce when making a transaction.

Rather than giving every method a nonce input arg, I've added a
WithNonceFunc option to pkg/client.Client. This is perhaps a
little strange but the cascade from a new argument was rather
large, and it seemed a little out of place as the other arguments
pertain to the action rather than the transaction containing it.
Can be switched if we'd prefer.
brennanjl pushed a commit that referenced this pull request Feb 26, 2024
This adds the ability to set a nonce when making a transaction.

Rather than giving every method a nonce input arg, I've added a
WithNonceFunc option to pkg/client.Client. This is perhaps a
little strange but the cascade from a new argument was rather
large, and it seemed a little out of place as the other arguments
pertain to the action rather than the transaction containing it.
Can be switched if we'd prefer.
jchappelow added a commit that referenced this pull request Feb 26, 2024
This adds the ability to set a nonce when making a transaction.

Rather than giving every method a nonce input arg, I've added a
WithNonceFunc option to pkg/client.Client. This is perhaps a
little strange but the cascade from a new argument was rather
large, and it seemed a little out of place as the other arguments
pertain to the action rather than the transaction containing it.
Can be switched if we'd prefer.
brennanjl pushed a commit that referenced this pull request Feb 26, 2024
This adds the ability to set a nonce when making a transaction.

Rather than giving every method a nonce input arg, I've added a
WithNonceFunc option to pkg/client.Client. This is perhaps a
little strange but the cascade from a new argument was rather
large, and it seemed a little out of place as the other arguments
pertain to the action rather than the transaction containing it.
Can be switched if we'd prefer.
brennanjl pushed a commit that referenced this pull request Feb 26, 2024
This adds the ability to set a nonce when making a transaction.

Rather than giving every method a nonce input arg, I've added a
WithNonceFunc option to pkg/client.Client. This is perhaps a
little strange but the cascade from a new argument was rather
large, and it seemed a little out of place as the other arguments
pertain to the action rather than the transaction containing it.
Can be switched if we'd prefer.
brennanjl pushed a commit that referenced this pull request Feb 26, 2024
This adds the ability to set a nonce when making a transaction.

Rather than giving every method a nonce input arg, I've added a
WithNonceFunc option to pkg/client.Client. This is perhaps a
little strange but the cascade from a new argument was rather
large, and it seemed a little out of place as the other arguments
pertain to the action rather than the transaction containing it.
Can be switched if we'd prefer.
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.

4 participants