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

improve performance of a crypt marshaller (ostracon#177) #2

Closed
Tracked by #1
0Tech opened this issue Nov 1, 2023 · 6 comments
Closed
Tracked by #1

improve performance of a crypt marshaller (ostracon#177) #2

0Tech opened this issue Nov 1, 2023 · 6 comments
Assignees
Labels
needs-triage priority/backlog Possibly useful, but not yet enough support to actually get it done.

Comments

@0Tech
Copy link

0Tech commented Nov 1, 2023

The subject of the triage:

@loloicci
Copy link

loloicci commented Nov 6, 2023

This is a PR for performance test and this PR says

Please note that it's not production ready quality

If we need this improvement in the main branch, it may need other PRs to improve it.

@loloicci loloicci changed the title Triage on ostracon#177 improve performance of a crypt marshaller (ostracon#177) Nov 15, 2023
@Mdaiki0730
Copy link
Member

Performance seems to be improved, so we can create a PR after considering what we need for main branch.

@loloicci
Copy link

@Mdaiki0730

Performance seems to be improved, so we can create a PR after considering what we need for main branch.

I think it is worth merging into the main form the view point of performance.
But, do you know why it's not production-ready quality? Or, do you know some improvements for it?

@Mdaiki0730
Copy link
Member

Mdaiki0730 commented Nov 17, 2023

@loloicci
The custom marshaler code is duplicated and needs to be packaged.

we need more test code for production.

As for the further tests mentioned in the PR, I don't know what to do.

@Mdaiki0730
Copy link
Member

By the way, the relevant parts of the crypto package have changed significantly in cometbft v0.38.0.
Looks like it was changed in tendermint#4950.
cometbft v0.38.0: https://github.com/cometbft/cometbft/blob/v0.38.0/crypto/ed25519/ed25519.go#L68-L70
PR: tendermint/tendermint#4950

Ostracon#177 doesn't seem to be necessary since it returns a byte slice directly instead of an amino-marshalled byte slice.

@0Tech 0Tech added priority/backlog Possibly useful, but not yet enough support to actually get it done. needs-triage and removed needs-triage labels Nov 29, 2023
@loloicci
Copy link

Thank you @Mdaiki0730 . I close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-triage priority/backlog Possibly useful, but not yet enough support to actually get it done.
Projects
None yet
Development

No branches or pull requests

3 participants