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

feat(solana): add nullable tag #3801

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RostarMarek
Copy link
Contributor

Fixes issue raised in this issue. The issue was in parsing the message. The question remaining is wheter any optional parameter in messages that are being parsed will be the same as here the new authority(1 byte to denote wheter the value is set and then bytes with value) or it is possible that the value bytes are not there for optional parameters. This fix works for the latter, if the former is true the parsing of is_optional should be changed.
This fix introduces a new flag nullable that for parsing purposes that reads and discards the value when the byte denotes that it is not set in the message.
One more thing to note when trying to update the fixtures of ui tests, the script moves the T3T1 part of the fixtures to the end of the file which creates a lot of line changes and it happened on multiple machines, therefore that change is not included int this PR, let me know wheter I should add it anyways.

@RostarMarek RostarMarek requested a review from matejcik as a code owner May 7, 2024 15:10
if property_template.is_optional:
is_included = True if reader.get() == 1 else False

if property_template.is_nullable:
is_included = True if reader.get() == 1 else False
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
is_included = True if reader.get() == 1 else False
is_included = reader.get() == 1

...

# (e.g. new_authority in Set Authority instruction) even if the is_included flag is not set.
# We can safely discard this value.
if reader.remaining_count() != 0:
property_template.parse(reader)
Copy link
Contributor

Choose a reason for hiding this comment

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

if we can safely discard, we should not be checking for remaining_count() here, right? consider a value that is "included" but then the data ends. we want the eof fail in that case

@matejcik
Copy link
Contributor

no, i'm pretty sure this is wrong.

SetAuthority layout:

    SetAuthority {
        /// The type of authority to update.
        authority_type: AuthorityType,
        /// The new authority
        new_authority: COption<Pubkey>,
    },

one non-optional field, one optional
Borsh serialization of optional fields:

if x.is_some() {
  repr(1 as u8)
  repr(x.unwrap() as ident)
} else {
  repr(0 as u8)
}

(that's exactly what "optional: true" does in our code)

if you set new_authority to null, this should be the layout:

u8 -> <SetAuthority instruction number>
u8 -> <authority type>
u8 -> 0 (new_authority not set

three bytes.
certainly not 0 and then trailing garbage

let's figure out what else could have been going wrong with the original report

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