[ts-sdk] Allow enums with no value, add a test for TransactionData BCS #1484
Conversation
Made minor edits inline
- renames MoveBCS to BCS - adds registerEnumType - more tests - enums - fills in license headers
- allows registering HEX address of any length
- increases code reusability in BCS
- adds toString('hex' | 'base64') to bcsWriter
| throw new Error(`Type ${elementType} is not registered`); | ||
| } | ||
| ): typeof BCS { | ||
| // OMITTING SAFETY CHECK TO ALLOW RECURSIVE DEFINITIONS |
There was a problem hiding this comment.
how important is this warning ? should we be worried about future issues or is it probably fine?
There was a problem hiding this comment.
It is a temporary solution, we'll see if we need to provide safety guarantees at early stages or later on. It unblocks having Type { val: Option<Type> }, which is good and allowed by BCS standard.
sdk/typescript/test/bcs/bcs.test.ts
Outdated
| @@ -3,13 +3,12 @@ | |||
|
|
|||
| import { BCS } from '../../src/bcs'; | |||
| import { Base64DataBuffer as B64 } from '../../src'; | |||
| // import { HexDataBuffer as HEX } from '../../src'; | |||
| import { BN } from 'bn.js'; | |||
| import * as BN from 'bn.js'; | |||
There was a problem hiding this comment.
nit: i didn't see anything else being used from BN, we could make this import more specific to avoid having to do new BN.BN(1)
sdk/typescript/test/bcs/bcs.test.ts
Outdated
| @@ -108,12 +107,122 @@ describe('Move BCS', () => { | |||
| expect(data.kitties).toHaveLength(2); | |||
| expect(data.owner).toEqual('00000000000000000000000000c0ffee'); | |||
| }); | |||
|
|
|||
| it('should de/ser TransactionData::Transfer', () => { | |||
| registerSuiCoreTypes(BCS); | |||
There was a problem hiding this comment.
when writing multiple tests with a common setup step, i think it's helpful to abstract out that step into its own setup function. This helps the tests to stay easy to read and write
here, that would look like:
describe('TransactionData', () => {
beforeEach(() => {
registerSuiCoreTypes(BCS);
});
it('should de/ser Self::Transfer', () => {
// same test but with the register types line removed
});
// other tests with same change
});
Although API is pretty straightforward, I suggest first deal with this one and then build a handy abstraction layer above.
TransactionDataforTransferserializes and deserializes correctly with one exception: addresses are encoded asVec<u8>on the Rust side, so technically they cannot be consideredaddressesin BCS. ButObjectIDis encoded as address.