BREAKING CHANGE: unexpected unpacked result in blockchain

[homura]

Description

Fixes #600

This pull request fixed the unpacked result of blockchain.*.unpack. It now matches the RPC type, which uses hex format instead of number or BI.

const { version } = blockchain.Transaction.unpack('0x...')

// before 
typeof version === 'number' // ❌ 
version === 0 // ❌ 

// after
typeof version === 'string' // ✅
version === '0x0' // ✅

This bug was caused by [email protected] at #413 but was never reported before #599 because the API Transaction.unpack or Block.unpack was so rarely used.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

• Unit tests involve both pack() and unpack() in base.blockchain

[vercel]

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

Name	Status	Preview	Updated (UTC)
lumos-website	✅ Ready (Inspect)	Visit Preview	Jan 9, 2024 5:42am

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (ccd8213) 83.84% compared to head (df10882) 87.19%.
Report is 1 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #601      +/-   ##
===========================================
+ Coverage    83.84%   87.19%   +3.34%     
===========================================
  Files          116      116              
  Lines        23583    23598      +15     
  Branches      2223     2418     +195     
===========================================
+ Hits         19773    20576     +803     
+ Misses        3770     2980     -790     
- Partials        40       42       +2     

Files	Coverage Δ
packages/base/src/blockchain.ts	98.88% <95.45%> (+0.49%)	⬆️

... and 10 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ccd8213...df10882. Read the comment docs.

[Keith-CY]

It works but the solution seems weird, it might be a historical issue but I still wonder why is number allowed when the risk of number is well-known

[homura]

It works but the solution seems weird, it might be a historical issue but I still wonder why is number allowed when the risk of number is well-known

I found that the problem originated from a large PR at #413. The PR removed the hex format conversion, but the reviewer didn't realize that the return value type had changed. This was because the types were annotated by being inferred from type Transaction = UnpackResult<typeof SomeCodec>. And there are rare scenarios in which Transaction.unpack is used during development because developers typically use the the Transaction with the default verbosity instead of the serialized one

[Keith-CY]

Approved but I'm still worried about the usage of number. My opinion might be kinda extreme, the number should be replaced by string and big number

[homura]

Approved but I'm still worried about the usage of number. My opinion might be kinda extreme, the number should be replaced by string and big number

It might be not consistent with some blockchain SDK, such as typechain. Lumos's current default number codec draws on some developer conventions from them