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

asset: update parsing to return error if unknown odd type encountered #557

Closed
wants to merge 1 commit into from

Conversation

Roasbeef
Copy link
Member

In this commit, as a follow up to the recent even/odd type split, we add some helper functions to assert that when we decode an asset TLV stream, we bail out if we find any unknown types.

In this commit, as a follow up to the recent even/odd type split, we add
some helper functions to assert that when we decode an asset TLV stream,
we bail out if we find any unknown types.
@Roasbeef Roasbeef requested a review from jharveyb October 10, 2023 00:41
@@ -30,6 +34,71 @@ const (
LeafAssetID LeafTlvType = 11
)

// KnownAssetLeafTypes is a set of all known leaf types.
var KnownAssetLeafTypes = fn.NewSet(
Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally we can set up some compile or init level thing to make sure that each time the set of types are updated, then this is as well.

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.

Nice, LGTM 🎉


// Erorr returns the error message for the ErrUnknownType.
func (e ErrUnknownType) Error() string {
return fmt.Errorf("unknown type %d", e.UnknownType).Error()
Copy link
Member

Choose a reason for hiding this comment

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

nit: why not just fmt.Sprintf()?

Copy link
Contributor

@jharveyb jharveyb left a comment

Choose a reason for hiding this comment

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

LGTM! Looks like something we should have across all the larger types, proofs etc.?

}

for _, testCase := range testCases {

Copy link
Contributor

Choose a reason for hiding this comment

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

linter fail on this extra newline.

@dstadulis
Copy link
Collaborator

!lightninglabs-deploy mute 720h00m

@dstadulis
Copy link
Collaborator

!lightninglabs-deploy mute 720h00m

@lightninglabs-deploy
Copy link

@Roasbeef, remember to re-request review from reviewers when ready

@dstadulis
Copy link
Collaborator

!lightninglabs-deploy mute 336h00m

@guggero
Copy link
Member

guggero commented Jul 19, 2024

Replaced by #1037.

@guggero guggero closed this Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

5 participants