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

refactor(InMemorySinger) - Implement a InMemorySigner::test method #12503

Open
wacban opened this issue Nov 22, 2024 · 4 comments
Open

refactor(InMemorySinger) - Implement a InMemorySigner::test method #12503

wacban opened this issue Nov 22, 2024 · 4 comments
Labels
C-good-first-issue Category: issues that are self-contained and easy for newcomers to work on. Near Core T-core Team: issues relevant to the core team

Comments

@wacban
Copy link
Contributor

wacban commented Nov 22, 2024

Description

There is a prevalent pattern in our test code:

            InMemorySigner::from_seed(account_id.clone(), KeyType::ED25519, account_id.as_ref());

It's quite verbose and annoying. The goal of this task is to implement a shorter InMemorySigner::test(account_id) that would do the above. Additionally please refactor a few usages of the old pattern with the new, shorter one. It should only be used in tests.

@wacban wacban added C-good-first-issue Category: issues that are self-contained and easy for newcomers to work on. T-core Team: issues relevant to the core team Near Core labels Nov 22, 2024
@mkamonMdt
Copy link
Contributor

Hi,
It seems to me that there already is something similar in near-primitives::test_utils called create_user_test_signer.
Screenshot from 2024-11-23 20-39-16

Screenshot from 2024-11-23 20-41-52

The above is used already in couple places.
Screenshot from 2024-11-23 20-42-57

Did you consider using it instead of creating new one? Also maybe returning Singer instead of InMemorySigner could come handy as it would simply #12472 for #1531. What do you think? I could implement that :)

@wacban
Copy link
Contributor Author

wacban commented Nov 25, 2024

free function

I'm against this though it's more of a preference than objective thing. My thinking is that as developer whenever I need to pass an InMemorySigner somewhere I can just look at that class and use the test method.

Signer

Hmm I would rather return the InMemorySigner but perhaps we can do both? e.g. test returning Self and test_signer returning Signer

I could implement that :)

Feel free to grab this task, thank you!

mkamonMdt added a commit to mkamonMdt/nearcore that referenced this issue Nov 25, 2024
Following the issue near#12503 the commit provides and factory methods
for testing purposes only. Use case examples are also provided.

The general idea is to wrap verbose but prevalent pattern of setting
up a test case with  calls.
mkamonMdt added a commit to mkamonMdt/nearcore that referenced this issue Nov 26, 2024
Following the issue near#12503 the commit provides and factory methods
for testing purposes only. Use case examples are also provided.

The general idea is to wrap verbose but prevalent pattern of setting
up a test case with  calls.
mkamonMdt added a commit to mkamonMdt/nearcore that referenced this issue Nov 27, 2024
Following the issue near#12503 the commit provides and factory methods
for testing purposes only. Use case examples are also provided.

The general idea is to wrap verbose but prevalent pattern of setting
up a test case with  calls.
mkamonMdt added a commit to mkamonMdt/nearcore that referenced this issue Nov 27, 2024
Following the issue near#12503 the commit provides and factory methods
for testing purposes only. Use case examples are also provided.

The general idea is to wrap verbose but prevalent pattern of setting
up a test case with  calls.
github-merge-queue bot pushed a commit that referenced this issue Nov 28, 2024
…12515)

Following the issue #12503 the commit provides and factory methods for
testing purposes only. Use case examples are also provided.

The general idea is to wrap verbose but prevalent pattern of setting up
a test case with calls.
@mkamonMdt
Copy link
Contributor

@wacban I could provide a followup commit with some more code refactor to use test/test_signer. Please let me know if you would like me to do it :)

@wacban
Copy link
Contributor Author

wacban commented Nov 28, 2024

@mkamonMdt That would be great, go for it, thank you!

mkamonMdt added a commit to mkamonMdt/nearcore that referenced this issue Nov 30, 2024
Two factory methods for InMemorySigner were provided in near#12503.
The following commit applies the methods broadly, reducing verbose
test code where possible.
mkamonMdt added a commit to mkamonMdt/nearcore that referenced this issue Dec 1, 2024
Two factory methods for InMemorySigner were provided in near#12503.
The following commit applies the methods broadly, reducing verbose
test code where possible.
github-merge-queue bot pushed a commit that referenced this issue Dec 2, 2024
…12537)

Two factory methods for InMemorySigner were provided in #12503. The
following commit applies the methods broadly, reducing verbose test code
where possible.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-good-first-issue Category: issues that are self-contained and easy for newcomers to work on. Near Core T-core Team: issues relevant to the core team
Projects
None yet
Development

No branches or pull requests

2 participants