Skip to content

Adds KdfResponses component#571

Merged
smk762 merged 28 commits intoinject-clean-electrumsfrom
responses-component
Aug 26, 2025
Merged

Adds KdfResponses component#571
smk762 merged 28 commits intoinject-clean-electrumsfrom
responses-component

Conversation

@smk762
Copy link
Contributor

@smk762 smk762 commented Aug 7, 2025

Pairs with https://github.com/gcharang/komodo-docs-revamp-2023/pull/194

  • Migrates KDF activation responses to use new KdfResponse component, which links by common key to a CodeGroup request (one to many).
  • adds validation script to identify errors or missing request/response pairs.

@smk762 smk762 requested a review from gcharang August 7, 2025 12:35
@shamardy shamardy self-requested a review August 14, 2025 14:51
@shamardy
Copy link

Great idea, just took a quick look at the PR and remembered this in KDF https://github.com/KomodoPlatform/komodo-defi-framework/blob/main/mm2src/mm2_test_helpers/src/for_tests.rs
So once we extract all the jsons, we can have them in KDF and use one source for both docs and KDF tests instead of our for_tests. Would it make more sense if we categorized the methods by wallets/swaps/etc.. in addition to v1 and v2? This way we can place them in the right place from KDF side and it might be easier to generate them in the right place in docs straight away.

Copy link

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

First review!

self.valid_response_types = {'success', 'error'}

# Valid request key pattern (alphanumeric, camelCase/PascalCase)
self.request_key_pattern = re.compile(r'^[A-Z][a-zA-Z0-9]*$')

Choose a reason for hiding this comment

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

nit: This doesn't allow camelCase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in cc71c88

Everything is PascalCase so dropped the camel, but made provision for numeric entry (e.g. 1InchMethods)

Comment on lines +421 to +422
parser.add_argument('--fix-minor-issues', action='store_true',
help='Automatically fix minor formatting issues')

Choose a reason for hiding this comment

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

Is there any usage of fix-minor-issues arg / flag? I can't find any

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope looks like a remnant. removed in e8f3759

f"Error reading file: {str(e)}"
))

def _validate_file_structure(self, file_path: Path, data: Dict, version: str):

Choose a reason for hiding this comment

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

version is never used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, removed in acfdaed

Comment on lines +161 to +164
"platform_coin": {
"platform": "ETH",
"ticker": "MATIC"
},

Choose a reason for hiding this comment

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

Is the response returned like this, I didn't look at kdf but I think you duplicated ticker here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Responses will be pruned and live-generated once #582 is wired up.

"title": "Transport error",
"notes": "Connection to ETH node failed",
"json": {
"error": "Transport error: JsonRpcError { client_info: \"coin: ETH\", request: JsonRpcRequest { jsonrpc: \"2.0\", id: \"9\", method: \"eth_getBalance\", params: [String(\"0x7Bc1bBDD6A0a722fC9bffc49c921B685ECB84b94\"), String(\"latest\")] }, error: Transport(\"All electrum servers unavailable!\") }"

Choose a reason for hiding this comment

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

Enable eth shouldn't have All electrum servers unavailable in errors :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Responses will be pruned and live-generated once #582 is wired up.

@github-actions
Copy link

github-actions bot commented Aug 22, 2025

Documentation Preview Links

Preview for commit: be1e274
Base URL: https://e57d324b.komodo-docs.pages.dev/en/docs

Changed Documentation Files

Commit: abc4d37 - Merge pull request #578 from KomodoPlatform/lint-fix-patches/responses-component

gcharang and others added 3 commits August 22, 2025 10:42
…nks, formatted md content, checked presence of file and dirs based on sidebar, checked presence of h1 in every file, update Komodo DeFi Framework methods table, adds/updates preview images when base is main
gcharang and others added 4 commits August 22, 2025 11:07
…nks, formatted md content, checked presence of file and dirs based on sidebar, checked presence of h1 in every file, update Komodo DeFi Framework methods table, adds/updates preview images when base is main
gcharang and others added 2 commits August 22, 2025 11:28
…nks, formatted md content, checked presence of file and dirs based on sidebar, checked presence of h1 in every file, update Komodo DeFi Framework methods table, adds/updates preview images when base is main
@smk762
Copy link
Contributor Author

smk762 commented Aug 25, 2025

Great idea, just took a quick look at the PR and remembered this in KDF https://github.com/KomodoPlatform/komodo-defi-framework/blob/main/mm2src/mm2_test_helpers/src/for_tests.rs So once we extract all the jsons, we can have them in KDF and use one source for both docs and KDF tests instead of our for_tests. Would it make more sense if we categorized the methods by wallets/swaps/etc.. in addition to v1 and v2? This way we can place them in the right place from KDF side and it might be easier to generate them in the right place in docs straight away.

I'm happy to follow whichever structure is most convenient for KDF team (and if required, make changes to mirror the same in docs repo).

@smk762 smk762 changed the base branch from inject/electrums to inject-clean-electrums August 26, 2025 05:53
@smk762 smk762 merged commit cd00d3f into inject-clean-electrums Aug 26, 2025
2 checks passed
@smk762 smk762 mentioned this pull request Aug 26, 2025
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.

3 participants