[xc-admin] Add encoder for governance messages #462
Conversation
guibescos
changed the title
| import { encodeExecutePostedVaa } from "../governance_payload/ExecutePostedVaa"; | ||
|
|
||
| test("GovernancePayload", (done) => { | ||
| test("GovernancePayload ser/de", (done) => { |
There was a problem hiding this comment.
I don't quite like this test as changing a payload/structure requires spending time on fixing it here.
specially that you go from "decode" to "encode" again. it means on change either
(1) someone should hand roll the struct manually (2) that someone should write down another structure and calls encode to get the new value.
I think it's better to encode and decode a struct. That's easier to modify/update.
There was a problem hiding this comment.
yeah agree with this comment. it's easier to update a struct in code than to update byte arrays
There was a problem hiding this comment.
I refactored to to struct -> encode -> bytes -> decode -> struct. Should I delete the hardcoded bytes arrays and make it struct -> encode -> decode -> struct ?
|
Can you add the tests to CI? |
| import { encodeExecutePostedVaa } from "../governance_payload/ExecutePostedVaa"; | ||
|
|
||
| test("GovernancePayload", (done) => { | ||
| test("GovernancePayload ser/de", (done) => { |
There was a problem hiding this comment.
yeah agree with this comment. it's easier to update a struct in code than to update byte arrays
| } | ||
|
|
||
| /** Encode ExecutePostedVaaArgs */ | ||
| export function encodeExecutePostedVaa( |
There was a problem hiding this comment.
software engineering comment: if you have functions like this that update a buffer and need to track an offset, generally a good engineering pattern is to make an object like a Writer which has a function like append that writes the bytes and tracks the offset in one shot
There was a problem hiding this comment.
Some explanation of why I did this:
- The underlying buffer-layout library uses this signature for encoding (
encode<T>(struct : T, buffer: Buffer) : number. it returns the offset as a return value). - I think the only place where I use this offset is for the header and the body of the message, so I don't think refactoring into a Writer is worth it right now.
There was a problem hiding this comment.
I updated encodeExecutePostedVaa to abstract away the buffer initialization and offset. So the signature for that top level method is encodeExecutePostedVaa( args : executePostedVaaArgs) : Buffer
Done |
| /** Encode ExecutePostedVaaArgs */ | ||
| export function encodeExecutePostedVaa(src: ExecutePostedVaaArgs): Buffer { | ||
| // PACKET_DATA_SIZE is the maximum transactin size of Solana, so our serialized payload will never be bigger than that | ||
| const buffer = Buffer.alloc(PACKET_DATA_SIZE); |
There was a problem hiding this comment.
this code comment seems wrong because the code doesn't enforce this size constraint. I could theoretically make src have a bunch of instructions in it such that the serialized version of the message is bigger than PACKET_DATA_SIZE.
There was a problem hiding this comment.
This part is a little awkward, I don't know the length before I encode it.
I'll try to improve the comment.
Add encoder for governance header and ExecutePostedVaa