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 MaxMetadataLength for credit class & credit batch metdata #520

Closed
clevinson opened this issue Sep 7, 2021 · 4 comments · Fixed by #540
Closed

Add MaxMetadataLength for credit class & credit batch metdata #520

clevinson opened this issue Sep 7, 2021 · 4 comments · Fixed by #540

Comments

@clevinson
Copy link
Member

Credit class & credit batches should have a max metadata length (similar to x/group module).

We should come up with a sensible max metadata limit, and also ensure documentation is updated to communicate that there is a limit to metadata size.

@clevinson
Copy link
Member Author

clevinson commented Sep 8, 2021

@aaronc suggested 256 bytes as a number that we should use for MaxMetadataLength. I think this makes sense assuming that this field is not storing raw metadata, but rather just content identifiers / hashes / URIs that identify metadata that lives somewhere off chain (or onchain but in the data module).

@aaronc Should we be more explicit about what the structure of that metadata looks like? Should it be a string following a URI format ([scheme]://[host]/[path]) ? Maybe not necessary in code, but perhaps in documentation?

@ruhatch ruhatch self-assigned this Sep 8, 2021
@ruhatch
Copy link
Contributor

ruhatch commented Sep 8, 2021

@clevinson do we want this to be implemented as a param? A comment in the group module suggests that that wanted to be migrated to a param later on:

// MaxMetadataLength defines the max length of the metadata bytes field
// for various entities within the group module
// TODO: This could be used as params once x/params is upgraded to use protobuf
const MaxMetadataLength = 255

@clevinson
Copy link
Member Author

I think we should probably hold off implementing as a param for now (similar to group module). In group we suggested doing that once we refactor how x/params works, and that's going to happen on a longer timeline.

@technicallyty
Copy link
Contributor

should we also charge gas based on the length of the metadata? in ethereum storing a 256-bit word costs around 20,000 gas. for our case i dont think it needs to be that high - but imo should have some sort of storage cost

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants