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

feat: add the query signers command #315

Merged
merged 8 commits into from
Apr 26, 2023
Merged

Conversation

rach-id
Copy link
Member

@rach-id rach-id commented Apr 25, 2023

Overview

Closes #292

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@rach-id rach-id added p2p p2p network related orchestrator orchestrator related labels Apr 25, 2023
@rach-id rach-id requested a review from evan-forbes as a code owner April 25, 2023 22:08
@rach-id rach-id self-assigned this Apr 25, 2023
@codecov-commenter
Copy link

codecov-commenter commented Apr 25, 2023

Codecov Report

Merging #315 (e5dd8d8) into main (59db7a2) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #315   +/-   ##
=======================================
  Coverage   43.42%   43.42%           
=======================================
  Files          26       26           
  Lines        1992     1992           
=======================================
  Hits          865      865           
  Misses       1005     1005           
  Partials      122      122           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

I feel like having existing relayers or orchestrators have an rpc to serve data might be something we want to pursue if this functionality is needed instead of spinning up a new in memory relayer each time we query a nonce, wdyt?

Comment on lines +63 to +71
stopFuncs := make([]func() error, 0)
defer func() {
for _, f := range stopFuncs {
err := f()
if err != nil {
logger.Error(err.Error())
}
}
}()
Copy link
Member

@evan-forbes evan-forbes Apr 26, 2023

Choose a reason for hiding this comment

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

[not blocking]

ideally we just call this later and pass the slice after we have appended to it. This should work, I think its just not ideal since multiple routines could touch that slice simultaneously. also, since we're already makeing a slice, then we might as well use the expected capacity

stopFuncs := make([]func() error, 0, 1)

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines +102 to +125
// creating the data store
dataStore := dssync.MutexWrap(ds.NewMapDatastore())

// creating the dht
dht, err := p2p.NewQgbDHT(cmd.Context(), h, dataStore, []peer.AddrInfo{}, logger)
if err != nil {
return err
}

// creating the p2p querier
p2pQuerier := p2p.NewQuerier(dht, logger)

nonce, err := parseNonce(ctx, appQuerier, args[0])
if err != nil {
return err
}
if nonce == 1 {
return fmt.Errorf("nonce 1 doesn't need to be signed. signatures start from nonce 2")
}

err = getSignaturesAndPrintThem(ctx, logger, appQuerier, tmQuerier, p2pQuerier, nonce)
if err != nil {
return err
}
Copy link
Member

Choose a reason for hiding this comment

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

how long should this take and how heavy is this operation? should we just query an existing relayer instead of spinning up one in memory and querying the dht?

Copy link
Member Author

Choose a reason for hiding this comment

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

It takes no time to do this. Yes, it would make sense to add an RPC endpoint to get this information. However, we don't have time right now to implement it and we need a way to know who signed or not. We can merge this and open an issue to have this as an endpoint, what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

okie dokie that's what I thought

@rach-id rach-id merged commit a30c81d into celestiaorg:main Apr 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
orchestrator orchestrator related p2p p2p network related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement a GetAttestationSignatures command
3 participants