Implemented the Message and MessageComputer classes #378
Conversation
src/message.ts
Outdated
| /** | ||
| * Number representing the message version. | ||
| */ | ||
| public version?: number; |
There was a problem hiding this comment.
Can be non-optional.
src/message.ts
Outdated
| this.data = options.data; | ||
| this.signature = options.signature; | ||
| this.address = options.address; | ||
| this.version = options.version ? options.version : DEFAULT_MESSAGE_VERSION; |
There was a problem hiding this comment.
Somehow unexpected when version will be set to 0 (but all right in practice, since there is no version 0). Maybe check for undefined instead of options.version?
(optional)
There was a problem hiding this comment.
As discussed, since version = 0 it's not valid it ok how it is.
There was a problem hiding this comment.
this.version = options.version || DEFAULT_MESSAGE_VERSION would be shorter
(optional)
src/message.ts
Outdated
| } | ||
|
|
||
| unpackMessage(packedMessage: { message: string; signature?: string; address?: string; version?: number }): Message { | ||
| let dataHex = this.trimHexPrefix(packedMessage.message); |
src/message.ts
Outdated
| let signature: Uint8Array | undefined = undefined; | ||
| if (packedMessage.signature) { | ||
| let signatureHex = this.trimHexPrefix(packedMessage.signature); | ||
| signature = Buffer.from(signatureHex, "hex"); | ||
| } |
There was a problem hiding this comment.
Can also be (proposal):
const signatureHex = this.trimHexPrefix(packedMessage.signature || "");
const signature = Buffer.from(signatureHex, "hex")
Will be an empty buffer if signature not provided, but that is maybe even better.
There was a problem hiding this comment.
Replaced with your proposal. Thanks!
| @@ -13,6 +13,9 @@ interface IConfig { | |||
| gasLimitPerByte: BigNumber.Value; | |||
| } | |||
|
|
|||
| /** | |||
| * Use this class to create both RelyedV1 and RelayedV2 transactions. | |||
src/interface.ts
Outdated
| } | ||
| } | ||
|
|
||
| export interface IMessage { |
There was a problem hiding this comment.
Maybe here we can live without the interface (related to the brainstorming on sdk-py).
Thus, maybe the DTO should suffice, for now? Then introduce an interface when needed? CC: @ccorcoveanu, as well.
There was a problem hiding this comment.
Removed the interface.
| const packedMessage = messageComputer.packMessage(message); | ||
| assert.deepEqual(packedMessage, { | ||
| address: "erd1qyu5wthldzr8wx5c9ucg8kjagg0jfs53s8nr3zpz3hypefsdd8ssycr6th", | ||
| message: "74657374", |
There was a problem hiding this comment.
And now, re-re-thinking about it 🙈
@ccorcoveanu, all right if we do it this way? That is, without the 0x prefix. Seems less cumbersome. And when people see signature they automatically think: well, that is hex, of course. And message is the same, for consistency. How do you feel about it?
For context: #375
There was a problem hiding this comment.
I like the no-0x version better
| return createKeccakHash("keccak256").update(bytesToHash).digest(); | ||
| } | ||
|
|
||
| serializeForSigningRaw(): Buffer { |
There was a problem hiding this comment.
I wonder where is this used.
There was a problem hiding this comment.
Apparently it's not used anywhere. Should I delete it?
There was a problem hiding this comment.
There was a problem hiding this comment.
Will not delete it.
| @@ -653,7 +653,7 @@ export class TransactionComputer { | |||
|
|
|||
| const serialized = JSON.stringify(plainTransaction); | |||
|
|
|||
| return new Uint8Array(Buffer.from(serialized)); | |||
| return Buffer.from(serialized); | |||
There was a problem hiding this comment.
Why this change? Won't it break compatibility for apps that strictly require Uint8Array?
I known we talk some time ago @andreibancioiu how using Buffer was not the best choice and about transitioning to Uint8Array - just trying to get up to speed if something changed along the way since then
There was a problem hiding this comment.
Well, in theory it would break compatibility, but this has not been yet released so there can't be any apps that use it. And since Buffer implements Uint8Array I assume it's all right.
There was a problem hiding this comment.
Indeed, TransactionComputer is new in v13. As of now, we've avoided the migration to Buffer in older components (we'll postpone it a bit more).
src/message.ts
Outdated
| this.data = options.data; | ||
| this.signature = options.signature; | ||
| this.address = options.address; | ||
| this.version = options.version ? options.version : DEFAULT_MESSAGE_VERSION; |
There was a problem hiding this comment.
this.version = options.version || DEFAULT_MESSAGE_VERSION would be shorter
(optional)
| const packedMessage = messageComputer.packMessage(message); | ||
| assert.deepEqual(packedMessage, { | ||
| address: "erd1qyu5wthldzr8wx5c9ucg8kjagg0jfs53s8nr3zpz3hypefsdd8ssycr6th", | ||
| message: "74657374", |
There was a problem hiding this comment.
I like the no-0x version better
Implemented the
MessageandMessageComputerclasses based on the sdk-specs.