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

feat: support json output for query signers command #338

Merged
merged 9 commits into from
May 2, 2023

Conversation

rach-id
Copy link
Member

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

Overview

Closes #332

The JSON file output is something like:

[
  {
    "evmAddress": "0x91DEd26b5f38B065FC0204c7929Da1b2A21877Ad",
    "signature": "d2862103cc5c27607b3fdd1c04bab52a921a837dc91748ddea9cf7b06fe0e10f568329e62eddcec25231216d4dcdd10941cab36db3ae7e838ef68606aca5ebef00",
    "signed": true
  },
  {
    "evmAddress": "0x966e6f22781EF6a6A82BBB4DB3df8E225DfD9488",
    "signature": "1b0aca14ba92e2ce0adcb708a1be1587864cfa93342a820e7809e519536cc51331d7d7c75c1f5015355998d124351bbd46c9f763b0898eb98e802a3797ccc0bb01",
    "signed": true
  },
  {
    "evmAddress": "0x3EE99606625E740D8b29C8570d855Eb387F3c790",
    "signature": "",
    "signed": false
  },
  {
    "evmAddress": "0x3d22f0C38251ebdBE92e14BBF1bd2067F1C3b7D7",
    "signature": "",
    "signed": false
  }
]

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 the p2p p2p network related label Apr 29, 2023
@rach-id rach-id requested a review from mojtaba-esk April 29, 2023 09:17
@rach-id rach-id self-assigned this Apr 29, 2023
@rach-id rach-id requested a review from evan-forbes as a code owner April 29, 2023 09:17
@codecov-commenter
Copy link

codecov-commenter commented Apr 29, 2023

Codecov Report

Merging #338 (0cad2db) into main (a929c3a) will increase coverage by 0.20%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #338      +/-   ##
==========================================
+ Coverage   43.56%   43.76%   +0.20%     
==========================================
  Files          26       26              
  Lines        1990     1990              
==========================================
+ Hits          867      871       +4     
+ Misses       1001      998       -3     
+ Partials      122      121       -1     

see 1 file with indirect coverage changes

Comment on lines 247 to 250
file, err := os.Create(outputFile)
if err != nil {
return errors.Wrap(err, "error creating file")
}
Copy link
Member

Choose a reason for hiding this comment

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

[blocking question]
this is likely fine for most cases, but does this require one file per query? should we instead use something like

file, err := os.OpenFile(logFilePath, os.O_WRONLY|os.O_CREATE|os.O_APPEND, 0o644)
if err != nil {
	return err
}

this way, we can continually append to a single file

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense to me, @mojtaba-esk what do you think? Which would make your life easier?

Copy link
Member

Choose a reason for hiding this comment

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

the above suggestion will work both ways I believe, where if we specify a different file name with each query, it will create different files. If you use the file name, only then will it append.

Its likely that the standard library is doing something similar under the hood of os.Create, but this is more explicit and safer

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm thinking about this, and i wonder if the output file, if we keep appending to it, will it be a correct json output?

{
   // first output
}
{
   // second output
}

Do we care about that?
@evan-forbes

Copy link
Member Author

Choose a reason for hiding this comment

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

Example output from updated implementation:

{
  "signatures": [
    {
      "evmAddress": "0x3EE99606625E740D8b29C8570d855Eb387F3c790",
      "signature": "",
      "signed": false
    },
    {
      "evmAddress": "0x3d22f0C38251ebdBE92e14BBF1bd2067F1C3b7D7",
      "signature": "fdf4bbb3cf7a709c507cbc61b50db49237f5e2d74989da9681a0b117e73568692d417965e86cdaf43968709aeb0f192089c1ef8f8fee7fa65f4f31b21ec2c2af01",
      "signed": true
    },
    {
      "evmAddress": "0x91DEd26b5f38B065FC0204c7929Da1b2A21877Ad",
      "signature": "d82b4b7d096a17eef02b650f658ed4e474464f42884bd6db9995e487e6596a5073f39a817cd0ee10bfaf4e9b6d4c558252e6747e8c52173797bb46f5f75b57fa00",
      "signed": true
    },
    {
      "evmAddress": "0x966e6f22781EF6a6A82BBB4DB3df8E225DfD9488",
      "signature": "7362e738b918cc3a899ca6b4c55ec94a1e33f03f90f6e1b452a89c527aa32d8a61de6dac396e51c58425c927abb5ced6e9a71a925e519b490e5b515f6396a6c201",
      "signed": true
    }
  ],
  "nonce": 11,
  "majority_threshold": 2863311532,
  "current_threshold": 3221225472,
  "can_relay": true
}
{
  "signatures": [
    {
      "evmAddress": "0x966e6f22781EF6a6A82BBB4DB3df8E225DfD9488",
      "signature": "e279a29f9ffb16fb814a828fbd06a75f5de31edb96c138995784a162cd61bb13167914babfb12e03609f8e27530ffeb7ae7e2b090d35234b122673971f4aca5501",
      "signed": true
    }
  ],
  "nonce": 2,
  "majority_threshold": 2863311532,
  "current_threshold": 4294967296,
  "can_relay": true
}

Choose a reason for hiding this comment

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

Thanks @sweexordious This output makes it easy to process with the knack-updater tool.
Just a question: we need to run the command once to get all the evm addresses and their signing status up to the time right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool 👍

we need to run the command once to get all the evm addresses and their signing status up to the time right?

You need to run it for different nonces. The way the QGB works is via having multiple attestations, referenced by their nonce, signed by the orchestrators (which are run by the validators).
Thus, we need to check each nonce apart to know who signed it and who didn't

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.

I'm thinking about this, and i wonder if the output file, if we keep appending to it, will it be a correct json output?

yeah good point, it is purely dependant on what @mojtaba-esk is okay with atm, but in the future we might want to add some delimiter to make it easier to parse. either that or use something simlar to json lines.

Copy link
Member Author

Choose a reason for hiding this comment

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

The delimiter is already \n, so we should be good

cmd/qgb/query/cmd.go Outdated Show resolved Hide resolved
mojtaba-esk
mojtaba-esk previously approved these changes May 2, 2023
Copy link

@mojtaba-esk mojtaba-esk left a comment

Choose a reason for hiding this comment

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

LGTM!

@rach-id rach-id requested review from evan-forbes and mojtaba-esk May 2, 2023 16:59
@rach-id rach-id merged commit 0c008bf into celestiaorg:main May 2, 2023
@rach-id rach-id deleted the query_json branch May 2, 2023 18:25
@rach-id rach-id restored the query_json branch May 2, 2023 18:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
p2p p2p network related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Json output of the query signers command
4 participants