Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Add authorize endpoint to chain connector plugin #8809

Merged
merged 3 commits into from
Aug 9, 2023

Conversation

shuse2
Copy link
Collaborator

@shuse2 shuse2 commented Aug 4, 2023

What was the problem?

This PR resolves #8484

How was it solved?

  • Add authorize endpoint
  • Remove password config from chain connector plugin

How was it tested?

  • Register chain connector plugin in example app
// package.json needs to be updated to include chain connector plugin as well
export const registerPlugins = (app: Application): void => {
	app.registerPlugin(new ChainConnectorPlugin(), { loadAsChildProcess: true });
};
  • Run the app
  • ./bin/run endpoint:invoke chainConnector_authorize '{"password": "1234", "enable": false}'

@shuse2 shuse2 requested review from ishantiw and Phanco August 4, 2023 10:51
@shuse2 shuse2 self-assigned this Aug 4, 2023
@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Merging #8809 (ad6a98b) into release/6.0.0 (f28783e) will decrease coverage by 0.01%.
The diff coverage is 90.00%.

❗ Current head ad6a98b differs from pull request most recent head 3794825. Consider uploading reports for the commit 3794825 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                @@
##           release/6.0.0    #8809      +/-   ##
=================================================
- Coverage          83.36%   83.35%   -0.01%     
=================================================
  Files                596      596              
  Lines              22402    22408       +6     
  Branches            3303     3304       +1     
=================================================
+ Hits               18676    18679       +3     
- Misses              3726     3729       +3     
Files Changed Coverage Δ
...ns/lisk-framework-chain-connector-plugin/src/db.ts 94.31% <80.00%> (-1.89%) ⬇️
...ain-connector-plugin/src/chain_connector_plugin.ts 82.59% <83.33%> (-0.52%) ⬇️
...k-framework-chain-connector-plugin/src/endpoint.ts 97.36% <100.00%> (+1.21%) ⬆️
...sk-framework-chain-connector-plugin/src/schemas.ts 100.00% <100.00%> (ø)

... and 9 files with indirect coverage changes

Copy link
Contributor

@ishantiw ishantiw left a comment

Choose a reason for hiding this comment

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

@shuse2
Copy link
Collaborator Author

shuse2 commented Aug 7, 2023

@ishantiw in that line, _submitCCU is already called in the try-catch, so if the key is not enabled, it will always be logged before that line?

@shuse2 shuse2 requested a review from ishantiw August 7, 2023 21:40
@ishantiw
Copy link
Contributor

ishantiw commented Aug 8, 2023

@shuse2 Oh I thought we are throwing error when key not enabled, I think its better to throw error and be logged here lisk-sdk/framework-plugins/lisk-framework-chain-connector-plugin/src/chain_connector_plugin.ts because if we only return the next line will assume that the CCU was sent and will save the last sent CCM where it was not really sent to the other chain. So in this case we can raise error and it will retry to create the CCU next time with the CCM that was not sent earlier

@shuse2
Copy link
Collaborator Author

shuse2 commented Aug 8, 2023

@ishantiw good point! updated the PR to throw and log 🙇

@shuse2 shuse2 enabled auto-merge (squash) August 9, 2023 10:00
@shuse2 shuse2 merged commit 1e9af20 into release/6.0.0 Aug 9, 2023
8 checks passed
@shuse2 shuse2 deleted the 8484-add-authorize-endpoint branch August 9, 2023 10:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants