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

BlockId implementation #66

Merged
merged 7 commits into from
May 17, 2023
Merged

BlockId implementation #66

merged 7 commits into from
May 17, 2023

Conversation

aaroncox
Copy link
Member

No description provided.

All instances of Checksum256 that reference a BlockId have been updated to use the new BlockId type.
@aaroncox aaroncox requested a review from jnordberg May 11, 2023 00:00
@@ -74,6 +75,10 @@ export interface BuiltinTypes {
'extended_asset?'?: ExtendedAsset
'extended_asset[]': ExtendedAsset[]
'extended_asset[]?'?: ExtendedAsset[]
block_id: BlockId
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should verify this to be sure but I don't think there is a block_id builtin type so this could end up causing inconsistencies when decoding with core vs nodeos

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if I'm looking in the right place, but I do see this:

https://github.com/AntelopeIO/leap/blob/bd75cf0e3b8a95296a47a1d073da328c2e8854a3/libraries/chain/eosio_contract_abi.cpp#L14

I can remove it if it's not a legit type - just not sure where exactly to find the builtins for nodeos.

Copy link
Collaborator

@jnordberg jnordberg May 11, 2023

Choose a reason for hiding this comment

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

Yeah hard to say, I checked now in the abieos lib and it doesn't define it. One way to know for sure is to define an ABI that depends on the "block_id_type" and ask nodeos/cleos to decode it and see if that works

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the the BlockId from the builtins and changed all the predefined types to use it instead of the alias 👍

@@ -119,3 +119,21 @@ export class Checksum160 extends Checksum {
return new Checksum160(digest)
}
}

export type BlockIdType = BlockId | Checksum256Type
export class BlockId extends Checksum256 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

BlockId isn't actually a checksum, it's derived from a checksum but BlockId.hash() doesn't have a meaning so I think we should just inherit from Struct and keep it in its own file. Also it would be neat if it had a from method that accepted a Checksum256 and a number (that could replace the .id getter in p2p/types.ts)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm having a hell of a time figuring out how the Struct would be built for this without using the Checksum 🤔

export class BlockId extends Struct {
    @Struct.field('bytes[]', {array: true}) declare array: Bytes[]
}

This isn't right, I think I need a UInt8array ti be stored on the struct to work like a checksum? Any pointers here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You'd set the field to something like @Struct.field('bytes') declare data: Bytes and then access the data.array. But actually thinking more about this now a better approach is to just implement a new AbiSerializable type for this case. Something like:

type BlockIdType = BlockId | BytesType | {blockNum: UInt32Type; checksum: Checksum256Type}

export class BlockId implements ABISerializableObject {
    static abiName = 'block_id_type' // eosio contract context defines this with a _type suffix for some reason

    static from(value: BlockIdType) {
        if (isInstanceOf(value, this)) {
            return value
        }
        if (Bytes.isBytes(value)) {
            return new this(Bytes.from(value).array)
        } else {
            return this.fromBlockChecksum(value.checksum, value.blockNum)
        }
    }

    static fromBlockChecksum(checksum: Checksum256Type, blockNum: UInt32Type): BlockId {
        // move helper from p2p here
    }

    readonly array: Uint8Array

    constructor(array: Uint8Array) {
        if (array.byteLength !== 32) {
            throw new Error(`BlockId size mismatch, expected 32 bytes got ${array.byteLength}`)
        }
        this.array = array
    }

    equals(other: BlockIdType): boolean {
        const self = this.constructor as typeof BlockId
        try {
            return arrayEquals(this.array, self.from(other).array)
        } catch {
            return false
        }
    }

    get hexString(): string {
        return arrayToHex(this.array)
    }

    toABI(encoder: ABIEncoder) {
        encoder.writeArray(this.array)
    }

    toString() {
        return this.hexString
    }

    toJSON() {
        return this.toString()
    }

    // add helper methods here
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Reason being that Bytes packs its own length as a varuint prefix while Checksum/BlockId doesn't

Copy link
Member Author

Choose a reason for hiding this comment

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

I have this all setup now and my tests are passing, but the p2p tests are now failing with "invalid type" 🤔

Is this because the serialized data in the unit tests no longer is valid for the tests, or did I miss something?

This error is so deep in the serializer, I'm a bit lost lol

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I forgot to add the fromABI to the template above.


    static fromABI(decoder: ABIDecoder) {
        return new this(decoder.readArray(32))
    }

Add that and see it works

Copy link
Member Author

Choose a reason for hiding this comment

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

That did it! Honestly I never would have come to that conclusion from the error. I've still got a lot to learn when it comes to the internals of the serializer!

@aaroncox aaroncox merged commit 18e9390 into master May 17, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants