proof+tapdb: refactor and fix TLV unknown odd types bug for backward compatibility #1337
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit makes use of the sqlc.embed() directive that allows for re-usable structs, which makes parsing with dedicated functions easier.
To avoid circular package dependencies in the next commits, we move the URLDecoder and URLEncoder functions from the address to the asset package. That way we'll be able to also use it from the proof package.
No point in returning a new meta, we can just allow setting the decimal display on the struct directly.
This fixes a "tlv stream is not canonical" error when calculating the meta hash of a meta reveal that has multiple unknown odd types that was caused by the map iterating over the types in a random order. To make such an error more apparent in the future, we log it when it occurs. This unfortunately means that our unknown odd types won't be fully backward compatible to v0.5.0 but only v0.5.1...
With this commit we add a new test file that will just read the test vectors and make sure they can be parsed and validated with the current version. The idea is that in the future we create a CI step that takes this backward_compat_test.go file and all test vector files, then copy them into a cloned directory of an older version of tapd we want to verify them against. Because of the fix in the commit right before this one, we can't actually do this yet... But we should achieve unknown odd type backward compatibility with v0.5.1-rc2 and later.
Pull Request Test Coverage Report for Build 13019619266Details
💛 - Coveralls |
Roasbeef
approved these changes
Jan 29, 2025
| MetaDataHash []byte | ||
| MetaDataType sql.NullInt16 | ||
| MetaDataBlob []byte | ||
| AssetsMetum AssetsMetum |
There was a problem hiding this comment.
Very cool, weird how it mangles the name here tho...
| @@ -306,59 +305,39 @@ func (m *MetaReveal) DecDisplayOption() (fn.Option[uint32], error) { | |||
|
|
|||
| // SetDecDisplay attempts to set the decimal display value in existing JSON | |||
| // metadata. It checks that the new metadata is below the maximum metadata size. | |||
| func (m *MetaReveal) SetDecDisplay(decDisplay uint32) (*MetaReveal, error) { | |||
There was a problem hiding this comment.
IIUC this was in place to not mutate the meta reveal receiver here. AFAICT, was mainly a defensive precaution.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extracted refactor and standalone fixes from #1335 so we can get them in earlier.