Skip to content

Conversation

@jayantk
Copy link
Contributor

@jayantk jayantk commented Aug 6, 2022

as it says. getting pretty easy at this point.

Copy link
Contributor

@tompntn tompntn left a comment

Choose a reason for hiding this comment

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

Nice!

Comment on lines +195 to +199
pyth_assert(
mapping_data.num_ < PC_MAP_TABLE_SIZE,
ProgramError::InvalidArgument,
)?;

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we could also make pyth_asset take an error message, which is logged if the assert fails.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the idiomatic way to do this would be have your own error type.
Currently we're just reproducing the C code, which always errors with ProgramError::InvalidArgument

Comment on lines +181 to +191
[x, y, z]
if valid_funding_account(x)
&& valid_signable_account(program_id, y, size_of::<pc_map_table_t>())
&& valid_signable_account(program_id, z, PC_PROD_ACC_SIZE as usize)
&& valid_fresh_account(z) =>
{
Ok([x, y, z])
}
_ => Err(ProgramError::InvalidArgument),
}?;

Copy link
Contributor

Choose a reason for hiding this comment

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

Minor point, but this block might be simpler if we seperated the validity checks from the match statment, and instead had short check_valid_* functions which returned errors if their check failed. We could then propagate these errors with the ? macro:

check_valid_funding_account(funding_account)?;
check_valid_signable_account(program_id, tail_mapping_account, size_of::<pc_map_table_t>())?;
check_valid_signable_account(program_id, new_product_account, PC_PROD_ACC_SIZE as usize)?;
check_valid_fresh_account(new_product_account)?

This also has the advantage in allowing us to return more specific error messages which we could then log and convert to ProgramError::InvalidArgument at the top-level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a good idea. i'll do it in a separate pr though.

case e_cmd_init_mapping: return ERROR_INVALID_ARGUMENT;
case e_cmd_add_mapping: return add_mapping( prm, ka );
case e_cmd_add_product: return add_product( prm, ka );
// init_mapping is implemented in Rust.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a copy paste typo

prod_account.magic_ = PC_MAGIC;
prod_account.ver_ = version;
prod_account.type_ = PC_ACCTYPE_PRODUCT;
prod_account.size_ = size_of::<pc_prod_t>() as u32;
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually use try_into().map_error()? instead of us when casting integer types


initialize_product_account(new_product_account, hdr.ver_)?;

let current_index = mapping_data.num_ as usize;
Copy link
Contributor

@majabbour majabbour Aug 8, 2022

Choose a reason for hiding this comment

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

We usually use try_into().map_error()? instead of us when casting integer types

.k1_
.copy_from_slice(&new_product_account.key.to_bytes())
}
mapping_data.num_ += 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually use try_into().map_error()? instead of us when casting integer types

@jayantk jayantk merged commit b39b536 into main Aug 8, 2022
@jayantk jayantk deleted the add_product branch August 8, 2022 16:24
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.

5 participants