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

feat: add built-in plugin signer #2

Merged
merged 11 commits into from
Apr 13, 2023
Merged

feat: add built-in plugin signer #2

merged 11 commits into from
Apr 13, 2023

Conversation

EthanYuan
Copy link
Owner

The following updates have been made to the payment plugin:

  • The signer plugin has been separated from the previous payment plugin.
  • An extra key has been added to identify whether the otx is an aggregated one or not. The key is OTX_IDENTIFYING_META_AGGREGATE_COUNT with a value of 0x10012.
  • The RPC has been modified from query_otx_by_id to query_otx_status_by_id to avoid directly retrieving the otx.
  • Some necessary refactoring has been done.

@EthanYuan EthanYuan requested a review from doitian April 12, 2023 09:36

pub fn get_or_insert_otx_id(&mut self) -> Result<H256, OtxFormatError> {
if let Some(key_pair) = self
.meta
Copy link
Collaborator

Choose a reason for hiding this comment

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

Performance suggestion: we can use map internally and serialize to key value pairs on serialization.

log::info!("on_new_open_tx, index otxs count: {:?}", context.otxs.len());
if let Ok(aggregate_count) = otx.get_aggregate_count() {
log::info!("aggregate count: {:?}", aggregate_count);
if aggregate_count == 1 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Signer should not depend on aggregate_count. If we need a signal to notify the signer, it's better to add a new key such as otx status, where PendingSignature means the transaction is ready to sign.

);
let _ = responder.send(MessageFromHost::Ok);
}
MessageFromPlugin::NewMergedOtx((merged_otx, otx_hashes)) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

As mentioned in https://github.com/EthanYuan/open-transaction-pool/pull/2/files#r1164822517 , a meta key status can eliminate many messages here. We should let the otx tells the next step instead of relying on the external states. If the plugin want to notify the next step, it just need to set a field in the otx and send it back to the pool.

Copy link
Collaborator

@doitian doitian left a comment

Choose a reason for hiding this comment

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

Added some comments but they can be addressed in the following PRs.

@EthanYuan
Copy link
Owner Author

Added some comments but they can be addressed in the following PRs.

Thanks! I'll address these comments in the following PRs.

@EthanYuan EthanYuan merged commit 12eda77 into main Apr 13, 2023
@EthanYuan EthanYuan deleted the plugin-sign branch April 13, 2023 03:48
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.

2 participants