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: photon rpc + new cli #594

Merged
merged 32 commits into from
Apr 12, 2024
Merged

feat: photon rpc + new cli #594

merged 32 commits into from
Apr 12, 2024

Conversation

sergeytimoshin
Copy link
Contributor

No description provided.

cli/README.md Outdated Show resolved Hide resolved
sergeytimoshin and others added 24 commits April 12, 2024 17:30
feat: sol decompression (#567)

* feat: sol de compression

* feat: JS compress and decompress lamports (#572)

---------

Co-authored-by: Swen Schäferjohann <[email protected]>

ci: add github ci syntax linter (#574)

chore: Remove patched arkworks crates (#575)

We are using only upstream 0.4.0 arkworks crates now, there is no need
to point to patched 0.3.0 crates.

feat: gnark circuits inclusion + non-inclusion 2-in-1 (#569)

* feat: inclusion + non-inclusion gnark circuits

* Refactor gnark server spawning and killing process.

add actions for initSolOmnibusAccount, compressLamports, decompressLamports + happy path tests (#578)

refactor: rename gnark proof inputs to plural form (#579)

* refactor: rename gnark proof inputs to plural form

* formatting

feat: add SOL compression and decompression cli commands (#580)

* Add SOL compression and decompression commands

* Use getTestRpc instead of Connection in decompress and compress cli cmds.

sync interface

rpc-interface coerce from photon response. open: tests

all endpoints except token

wip: debug delay in photon indexing

wip: RPC tests working. debug: merkleproof, notest: token

wip: add sleep

wip: fix cli build

fresh install changes gnark-prover

merkleproof confirmed for 2 consecutive leaves

wip

wip: built

rm compress test dupe
Copy link
Contributor

@SwenSchaeferjohann SwenSchaeferjohann left a comment

Choose a reason for hiding this comment

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

lgtm!

@SwenSchaeferjohann
Copy link
Contributor

SwenSchaeferjohann commented Apr 12, 2024

edit: nice, all have been resolved. thx

3 caveats:

  • cli pnpm test doesn't include test-prove currently.
    "test": "pnpm kill && pnpm test-cli && pnpm test-utils && pnpm test-create-mint && pnpm test-mint-to && pnpm test-transfer && pnpm test-balance && pnpm test-compress-sol && pnpm test-decompress-sol",

  • also in cli package.json, we should run pnpm test-init-sol-pool (not included currently) before pnpm test-compress-sol (order important)

  • prover.js:browser test is disabled (likely browser dependency version mismatch, IMO non-critical and can be fixed and enabled again in a follow up PR)

The `test` script in the package.json file was updated to include `test-prove` and `test-init-sol-pool`.
@SwenSchaeferjohann
Copy link
Contributor

summary of changes

  • Fixes CLI: commands and tests are working properly and it also runs in CI again
  • SDK and CLI use Rpc() under the hood now
  • Test-validator correctly starts up gnark-prover and photon indexer servers by default, with opt-out capabilities
    • This is used in the sdk and cli tests

gnark-prover/main.go Outdated Show resolved Hide resolved
var circuitDir = context.String("circuit-dir")
var inclusion = context.Bool("inclusion")
var nonInclusion = context.Bool("non-inclusion")
return prover.GetKeys(circuitDir, inclusion, nonInclusion)
Copy link
Contributor

Choose a reason for hiding this comment

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

does this enable a server to be instantiated with both inclusion, combined and non-inclusion proving keys at the same time?

Copy link
Contributor

Choose a reason for hiding this comment

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

combined is missing right?

@@ -397,25 +387,25 @@ func runCli() {
break
}
}
} else if circuit == "combined" {
} else if context.Bool("inclusion") && context.Bool("non-inclusion") {
Copy link
Contributor

@ananas-block ananas-block Apr 12, 2024

Choose a reason for hiding this comment

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

is the idea that if both inclusion and non-inclusion flags are provided the combined circuit is used?
idk whether we use this part of the go cli at all but couldn't we ditch providing flags all together and just match the right circuit depending on the inputs like we discussed for the server?
(if we don't use this code just put a todo above)

Copy link
Contributor

@SwenSchaeferjohann SwenSchaeferjohann Apr 12, 2024

Choose a reason for hiding this comment

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

i think it's good to have to specify the circuit flag, not just via overloading the call signature. it's a more explicit API that way. will also be useful when we add more other circuits in the future

},
&cli.BoolFlag{Name: "inclusion", Usage: "Run inclusion circuit", Required: false},
&cli.BoolFlag{Name: "non-inclusion", Usage: "Run non-inclusion circuit", Required: false},
&cli.BoolFlag{Name: "combined", Usage: "Run combined circuit", Required: false},
Copy link
Contributor

@ananas-block ananas-block Apr 12, 2024

Choose a reason for hiding this comment

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

There are a lot of combined provingkeys. Thus, I think for the server it makes sense to keep the combined flag so that we have the option to load the inclusion, non-inclusion and combined subsets of provingkeys separately

Copy link
Contributor

Choose a reason for hiding this comment

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

i think this is an implementation detail that we can address in a follow-up pr. wdyt?

@SwenSchaeferjohann SwenSchaeferjohann merged commit 72c4439 into main Apr 12, 2024
7 checks passed
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.

4 participants