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

Add read input commands to rollups-cli #170

Merged
merged 2 commits into from
Dec 5, 2023
Merged

Conversation

GMKrieger
Copy link
Contributor

@GMKrieger GMKrieger added the #feat:rollups-cli Feature: cartesi-rollups-cli label Nov 23, 2023
@GMKrieger GMKrieger self-assigned this Nov 23, 2023
@gligneul gligneul requested a review from a team November 23, 2023 19:33
Copy link
Contributor

@gligneul gligneul left a comment

Choose a reason for hiding this comment

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

Looks good! Just minor comments.

pkg/graphqlutil/generate/input.graphql Outdated Show resolved Hide resolved
pkg/graphqlutil/generate/genqlient.yaml Outdated Show resolved Hide resolved
pkg/graphqlutil/generate/schema.graphql Outdated Show resolved Hide resolved
pkg/graphqlutil/main.go Outdated Show resolved Hide resolved
cmd/cartesi-rollups-cli/root/read/read.go Outdated Show resolved Hide resolved
cmd/cartesi-rollups-cli/root/read/input/input.go Outdated Show resolved Hide resolved
cmd/cartesi-rollups-cli/root/read/input/input.go Outdated Show resolved Hide resolved
cmd/cartesi-rollups-cli/root/read/input/input.go Outdated Show resolved Hide resolved
cmd/cartesi-rollups-cli/root/read/input/input.go Outdated Show resolved Hide resolved
cmd/cartesi-rollups-cli/root/read/inputs/inputs.go Outdated Show resolved Hide resolved
@GMKrieger GMKrieger force-pushed the feature/rollups-cli-read branch 3 times, most recently from fc3aeb5 to fa303d5 Compare November 27, 2023 17:08
pkg/readerclient/input.go Outdated Show resolved Hide resolved
pkg/readerclient/input.go Outdated Show resolved Hide resolved
Base automatically changed from feature/rollups-cli to next November 27, 2023 17:36
@GMKrieger GMKrieger force-pushed the feature/rollups-cli-read branch 3 times, most recently from d46741b to dd491f6 Compare November 28, 2023 13:24
Copy link
Contributor

@gligneul gligneul left a comment

Choose a reason for hiding this comment

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

Looks good; there are just a few minor comments left. I will remove the draft status.

cmd/cartesi-rollups-cli/root/read/input/input.go Outdated Show resolved Hide resolved
cmd/cartesi-rollups-cli/root/read/inputs/inputs.go Outdated Show resolved Hide resolved
pkg/readerclient/input.go Outdated Show resolved Hide resolved
@gligneul gligneul marked this pull request as ready for review November 28, 2023 14:10
@gligneul gligneul requested a review from a team November 28, 2023 14:11
pkg/readerclient/input.go Outdated Show resolved Hide resolved
@GMKrieger GMKrieger force-pushed the feature/rollups-cli-read branch 2 times, most recently from 0dea90e to 576376b Compare November 28, 2023 16:41
pkg/readerclient/input.go Outdated Show resolved Hide resolved
gligneul
gligneul previously approved these changes Nov 28, 2023
Copy link
Contributor

@gligneul gligneul left a comment

Choose a reason for hiding this comment

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

It is broken:

cartesi-rollups-cli read inputs
[91 10 32 32 32 32 123 10 32 32 32 32 32 32 32 32 34 105 110 100 101 120 34 58 32 48 44 10 32 32 32 32 32 32 32 32 34 115 116 97 116 117 115 34 58 32 34 65 67 67 69 80 84 69 68 34 44 10 32 32 32 32 32 32 32 32 34 109 115 103 83 101 110 100 101 114 34 58 32 34 48 120 102 51 57 102 100 54 101 53 49 97 97 100 56 56 102 54 102 52 99 101 54 97 98 56 56 50 55 50 55 57 99 102 102 102 98 57 50 50 54 54 34 44 10 32 32 32 32 32 32 32 32 34 116 105 109 101 115 116 97 109 112 34 58 32 49 55 48 49 51 55 55 56 56 56 44 10 32 32 32 32 32 32 32 32 34 98 108 111 99 107 78 117 109 98 101 114 34 58 32 53 54 44 10 32 32 32 32 32 32 32 32 34 112 97 121 108 111 97 100 34 58 32 34 48 120 54 56 54 57 34 10 32 32 32 32 125 10 93]

Copy link
Contributor

@torives torives left a comment

Choose a reason for hiding this comment

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

This is looking great! I've only left some smaller suggestions regarding documentation and naming things.

cmd/cartesi-rollups-cli/root/read/input/input.go Outdated Show resolved Hide resolved
cmd/cartesi-rollups-cli/root/read/inputs/inputs.go Outdated Show resolved Hide resolved
api/schema.graphql Outdated Show resolved Hide resolved
api/schema.graphql Outdated Show resolved Hide resolved
api/schema.graphql Outdated Show resolved Hide resolved
pkg/readerclient/input.go Outdated Show resolved Hide resolved
pkg/readerclient/input.go Outdated Show resolved Hide resolved
pkg/readerclient/main.go Outdated Show resolved Hide resolved
@GMKrieger GMKrieger force-pushed the feature/rollups-cli-read branch 2 times, most recently from 321ba81 to 778d3c5 Compare December 1, 2023 18:10
@GMKrieger
Copy link
Contributor Author

Me and @gligneul discussed and decided it was best to remove the first parameter from the GetInputs function, as this a debug tool that doesn't need pagination. Now, it returns all inputs ordered by index.

gligneul
gligneul previously approved these changes Dec 4, 2023
@gligneul gligneul dismissed their stale review December 4, 2023 19:54

The merge-base changed after approval.

Base automatically changed from next to main December 4, 2023 20:31
torives
torives previously approved these changes Dec 5, 2023
@gligneul
Copy link
Contributor

gligneul commented Dec 5, 2023

Could you add an entry in the CHANGELOG?

@GMKrieger GMKrieger merged commit 5a4684e into main Dec 5, 2023
5 checks passed
@GMKrieger GMKrieger deleted the feature/rollups-cli-read branch December 5, 2023 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
#feat:rollups-cli Feature: cartesi-rollups-cli
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants