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

Make VarInt to be restricted to u32 in length #46

Merged
merged 1 commit into from
Jul 12, 2023
Merged

Make VarInt to be restricted to u32 in length #46

merged 1 commit into from
Jul 12, 2023

Conversation

dr-orlovsky
Copy link
Member

@dr-orlovsky dr-orlovsky commented Jul 12, 2023

This is a quickfix for LNP-BP/client_side_validation#133 to make the things working before we will find a long-term solution at the type level.

For now, the guarantee that the problem doesn't happen elsewhere will be to test all strict types generated lib ids in all consensus repos against predefined constants (they are already in place) in wasm32-run tests.

In terms of bitcoin consensus, there are no practical reasons to believe that bitcoin scripts or the number of tx inputs/outputs will ever exceed 4 billions per tx/script; so it Is safe. Anyway, with rust we can't do anything better.

@dr-orlovsky dr-orlovsky added *compatibility* Issues affecting compatibility and interoperability *consensus* Issues affecting distributed concensus labels Jul 12, 2023
@dr-orlovsky dr-orlovsky added this to the v0.10.x milestone Jul 12, 2023
Copy link
Member

@cryptoquick cryptoquick left a comment

Choose a reason for hiding this comment

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

I think this is the best way.

Copy link
Member

@crisdut crisdut left a comment

Choose a reason for hiding this comment

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

Great work!

After merge, we need release a new version to update other stl, like rgb_contracts and rgb_std_stl.

@dr-orlovsky
Copy link
Member Author

dr-orlovsky commented Jul 12, 2023

Downstream the stl change without a release since they are generated from the rust types and are not hardcoded. The IDs will change - thus the existing tests will fail, but the crates will still compile and work in the correct way (with the new types).

@dr-orlovsky
Copy link
Member Author

So I mean we will need to update them - but just eventually, as a part of the planned release process (maybe alongside adding WASM32 CI running tests).

@dr-orlovsky dr-orlovsky merged commit bc8b5e0 into master Jul 12, 2023
36 checks passed
@dr-orlovsky dr-orlovsky deleted the usize branch July 12, 2023 19:21
@BP-WG BP-WG deleted a comment from adutravx Jul 12, 2023
@crisdut
Copy link
Member

crisdut commented Jul 12, 2023

So I mean we will need to update them - but just eventually, as a part of the planned release process (maybe alongside adding WASM32 CI running tests).

Sure, I can help you with this.

@crisdut
Copy link
Member

crisdut commented Jul 12, 2023

Downstream the stl change without a release since they are generated from the rust types and are not hardcoded. The IDs will change - thus the existing tests will fail, but the crates will still compile and work in the correct way (with the new types).

I'm not sure with I understand.

@dr-orlovsky
Copy link
Member Author

Doesn't matter anymore: I already updated & released all crates downstream till RGB Core (which is pending another fix)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
*compatibility* Issues affecting compatibility and interoperability *consensus* Issues affecting distributed concensus
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants