Implementing get_size for everything

[blockiosaurus]

Fully implementing get_size and switching to that over serialized data size.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
mpl-core-js-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 30, 2024 10:04pm

[nhanphan]

Is there a way to add a test to check the sizes are actually what we expect? (Eg they match with the plugin header)

[blockiosaurus]

You're right. We should probably have serialization size tests for every plugin.

[danenbm]

Looked at the trait implementations. Still need to look at processor code.

[danenbm]

is this really 1 or since there's no None, is it each plugin's initial size?

1 + match self {
    Plugin::Royalties(royalties) => royalties.get_initial_size()
    ...

[blockiosaurus]

There's still one byte required for the plugin enum discriminator.

[danenbm]

Right but there's no default value that is only the descriminator, so there's no time you could ever use BASE_LEN and it would be correct, right?

Whereas for example Attribute, the BASE_LEN would be the length of an empty key/value pair and you could call it for a default value.

I can't think of a good solution for this except for adding a None variant, which is a contrived solution as there's not an actual reason to add it

[blockiosaurus]

Ah yeah I see what you mean. Maybe BASE_LEN should be private or not required?

[danenbm]

I like that idea of not having BASE_LEN as a public trait const, and then people should only ever use len() which would work for empty or non-empty items. Like len() should always be trustable.

[blockiosaurus]

It's a little annoying when we have multi-level data blobs, still. Because with no public BASE_LEN you need to hard code the underlying type's size.

[danenbm]

Looks good but I agree with both of you we should add that testing now.

[github-actions]

Benchmark

Benchmark suite	Current: 33b5220	Previous: 87e2e35	Ratio
CU: create a new, empty asset	11182 Compute Units	11082 Compute Units	1.01
Space: create a new, empty asset	91 Bytes	91 Bytes	1
CU: create a new, empty asset with empty collection	22655 Compute Units	22555 Compute Units	1.00
Space: create a new, empty asset with empty collection	91 Bytes	91 Bytes	1
CU: create a new asset with plugins	35091 Compute Units	40123 Compute Units	0.87
Space: create a new asset with plugins	194 Bytes	194 Bytes	1
CU: create a new asset with plugins and empty collection	41167 Compute Units	46145 Compute Units	0.89
Space: create a new asset with plugins and empty collection	194 Bytes	194 Bytes	1
CU: list an asset	28041 Compute Units	30237 Compute Units	0.93
CU: sell an asset	39312 Compute Units	38699 Compute Units	1.02
CU: list an asset with empty collection	35599 Compute Units	37795 Compute Units	0.94
CU: sell an asset with empty collection	51640 Compute Units	51027 Compute Units	1.01
CU: list an asset with collection royalties	36336 Compute Units	38499 Compute Units	0.94
CU: sell an asset with collection royalties	56565 Compute Units	55952 Compute Units	1.01
CU: transfer an empty asset	6420 Compute Units	6320 Compute Units	1.02
CU: transfer an empty asset with empty collection	9018 Compute Units	8918 Compute Units	1.01
CU: transfer an asset with plugins	14719 Compute Units	14604 Compute Units	1.01
CU: transfer an asset with plugins and empty collection	17317 Compute Units	17202 Compute Units	1.01

This comment was automatically generated by workflow using github-action-benchmark.

[danenbm]

Overall looks great and love the testing, but I have one nagging concern about BASE_LEN (see comments)