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

rpc: unconstrain meta field for MintAssets #559

Merged
merged 1 commit into from
Oct 11, 2023
Merged

rpc: unconstrain meta field for MintAssets #559

merged 1 commit into from
Oct 11, 2023

Conversation

Roasbeef
Copy link
Member

In this commit, we stop trying to strictly parse the meta field. Instead, users can set it to w/e they want, with the default being opaque. This lets users start to add structure to the meta field from day one. One example is a set meta with fields that dictate how the asset unit is to be displayed. This can be used to do things like leverage the current fix point format to add decimal places for display.

@jharveyb
Copy link
Collaborator

The underlying proof.MetaReveal.MetaType field is uint8, but the matching field on the RPC request is int32 - we should still clamp it to uint8 before conversion.

@Roasbeef
Copy link
Member Author

but the matching field on the RPC request is int32 - we should still clamp it to uint8 before conversion.

Aren't we effectively doing that just with the cast?

https://go.dev/play/p/sSqSvPQEDi-

@guggero
Copy link
Member

guggero commented Oct 11, 2023

Aren't we effectively doing that just with the cast?

Yes, but that would just truncate the value and lead to unexpected behavior. I think what @jharveyb meant was to return an error if the value was bigger than math.MaxUint8.

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

LGTM pending the mentioned check for value overflow.

rpcserver.go Show resolved Hide resolved
@jharveyb
Copy link
Collaborator

Aren't we effectively doing that just with the cast?

Yes, but that would just truncate the value and lead to unexpected behavior. I think what @jharveyb meant was to return an error if the value was bigger than math.MaxUint8.

Or negative

@guggero
Copy link
Member

guggero commented Oct 11, 2023

Ah, I guess we should at least make it uint32 on the RPC level then.

@jharveyb
Copy link
Collaborator

Yeah I could find where it was defined as int32, just that the concrete type is an enum, but I would expect that to be uint32 and not int32 anyways.

Also may want a test to make sure that the same strict meta type handling isn't also present elsewhere in the minting logic.

@guggero
Copy link
Member

guggero commented Oct 11, 2023

Right, I forgot it was an enum. Looks like gRPC by default uses int32 even though that doesn't make that much sense. Don't think there's a way to influence that. So just over- and underflow checks should be enough.

@Roasbeef Roasbeef force-pushed the unleash-meta branch 2 times, most recently from 6bb73cc to f65d7f0 Compare October 11, 2023 20:32
In this commit, we stop trying to strictly parse the meta field.
Instead, users can set it to w/e they want, with the default being
opaque. This lets users start to add structure to the meta field from
day one. One example is a set meta with fields that dictate how the
asset unit is to be displayed. This can be used to do things like
leverage the current fix point format to add decimal places for display.
@Roasbeef Roasbeef merged commit 12f5c3a into main Oct 11, 2023
14 checks passed
@guggero guggero deleted the unleash-meta branch October 11, 2023 21:09
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.

None yet

3 participants