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_: set default node config values for frontend when doing local pair sync #5080

Merged
merged 1 commit into from
Apr 25, 2024

Conversation

qfrank
Copy link
Contributor

@qfrank qfrank commented Apr 22, 2024

Because mobile won't pass NetworkID/UpstreamConfig/Datadir anymore when doing local pair sync, backend will set the default value instead.

@qfrank qfrank self-assigned this Apr 22, 2024
@qfrank qfrank requested a review from Samyoul April 22, 2024 12:42
@status-im-auto
Copy link
Member

status-im-auto commented Apr 22, 2024

Jenkins Builds

Click to see older builds (9)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 55d19c8 #1 2024-04-22 12:44:43 ~4 min linux 📦zip
✔️ 55d19c8 #1 2024-04-22 12:46:32 ~6 min android 📦aar
✔️ 55d19c8 #1 2024-04-22 12:47:06 ~6 min ios 📦zip
✖️ 55d19c8 #1 2024-04-22 13:22:05 ~41 min tests 📄log
✖️ 55d19c8 #2 2024-04-22 14:56:54 ~39 min tests 📄log
✔️ f4f986d #2 2024-04-24 03:39:57 ~2 min linux 📦zip
✔️ f4f986d #2 2024-04-24 03:40:37 ~3 min ios 📦zip
✔️ f4f986d #2 2024-04-24 03:42:36 ~5 min android 📦aar
✖️ f4f986d #3 2024-04-24 04:19:47 ~42 min tests 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 5532ec5 #3 2024-04-24 08:34:12 ~2 min linux 📦zip
✔️ 5532ec5 #3 2024-04-24 08:34:14 ~2 min android 📦aar
✔️ 5532ec5 #3 2024-04-24 08:35:27 ~3 min ios 📦zip
✖️ 5532ec5 #4 2024-04-24 09:15:15 ~43 min tests 📄log
✔️ 72c7bdd #4 2024-04-25 01:47:20 ~2 min android 📦aar
✔️ 72c7bdd #4 2024-04-25 01:48:19 ~3 min ios 📦zip
✔️ 72c7bdd #4 2024-04-25 01:48:44 ~3 min linux 📦zip
✔️ 72c7bdd #5 2024-04-25 02:25:12 ~40 min tests 📄log

Copy link
Member

@Samyoul Samyoul left a comment

Choose a reason for hiding this comment

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

Thank you for this Frank.

@qfrank qfrank force-pushed the fix/local_pairing_network_id branch 2 times, most recently from f4f986d to 5532ec5 Compare April 24, 2024 08:31
@qfrank qfrank changed the title fix_: Error: Field validation for 'NetworkID' failed on the 'required' tag refactor_: set default node config values for frontend when doing local pair sync Apr 24, 2024
@qfrank qfrank requested a review from Samyoul April 24, 2024 08:44
Copy link
Member

@Samyoul Samyoul left a comment

Choose a reason for hiding this comment

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

Great fix. Thank you very much Frank 💪

Copy link
Contributor

@igor-sirotin igor-sirotin left a comment

Choose a reason for hiding this comment

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

Oh, I totally forgot that we also pass nodeConfig from client in local pairing.

Maybe instead of this quick fix it's better to remove nodeConfig parameter from local pairing API and take the default config from status-go?

There's intention to remove direct using of nodeConfig by client apps.
This is already done for create/restore accounts on mobile. Desktop is on the way: status-im/status-desktop#14139

@qfrank
Copy link
Contributor Author

qfrank commented Apr 25, 2024

Maybe instead of this quick fix it's better to remove nodeConfig parameter from local pairing API and take the default config from status-go?

I planned to merge this small PR first, and do another one to remove @igor-sirotin

@qfrank qfrank force-pushed the fix/local_pairing_network_id branch from 5532ec5 to 72c7bdd Compare April 25, 2024 01:44
@qfrank qfrank merged commit 90b18d4 into develop Apr 25, 2024
7 checks passed
@qfrank qfrank deleted the fix/local_pairing_network_id branch April 25, 2024 03:51
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.

None yet

4 participants