nervosnetwork/ckb-std

Document `SysError::LengthNotEnough` intended usage

Opened this issue ยท 9 comments

Following @XuJiandong advice:

The load_cell_data function can load all cell data at once. However, the cell data might be very large, potentially consuming all available memory in certain scenarios. It is recommended to use the following low-level syscall to load only a portion of the cell data

I switched iCKB load_cell_data calls from the highlevel to syscall. At that point all the contracts stopped validating, completely.

After too much digging I understood theSysError::LengthNotEnough lifecycle in correlation to syscalls from load_data implementation. I did not found any other documentation except that private function, not even in the relative RFC.

Please, could you update the syscall documentation with this crucial information about SysError::LengthNotEnough?

Keep up the great work ๐Ÿ’ช
Phroi

The document is here:

/// Buffer length is not enough, error contains actual data length

@XuJiandong that's way too little and way too hidden to call it documentation.

I would understand if Capsule 0.10.5 04fd58c integrated those nice comments on errors, but it still generates the following error.rs:`

use ckb_std::error::SysError;

/// Error
#[repr(i8)]
pub enum Error {
    IndexOutOfBound = 1,
    ItemMissing,
    LengthNotEnough,
    Encoding,
    // Add customized errors here...
    MyError,
}

impl From<SysError> for Error {
    fn from(err: SysError) -> Self {
        use SysError::*;
        match err {
            IndexOutOfBound => Self::IndexOutOfBound,
            ItemMissing => Self::ItemMissing,
            LengthNotEnough(_) => Self::LengthNotEnough,
            Encoding => Self::Encoding,
            Unknown(err_code) => panic!("unexpected sys error {}", err_code),
        }
    }
}

Also I would recommend for every low level syscall to document this particular behavior

The capsule is deprecated and now we're migrating projects to: https://github.com/cryptape/ckb-script-templates

Capsule deprecated, really?!

As you may know, iCKB just got an internal code review and I'm sure that the reviewers would have told me right away if the Nervos standard library on which iCKB is built on top is deprecated.

Also until now no deprecation notice appears on both capsule and ckb-std Readme.

While I understand that it's in the process of being switched out (not sure why), it currently doesn't seem deprecated.

Additionally, these docs also helps understand better code that is alredy deployed.

Love & Peace, Phroi

I'm sure that the reviewers would have told me right away if the Nervos standard library on which iCKB is built on top is deprecated.

This is not correct. The capsule is just a tool and doesn't affect the final on-chain script at all. Capsule doesn't include ckb-std or any other on-chain script libraries.

Wow, what a relief!! Then my issue still remains with ckb-std, which is not gonna be deprecated, right? ๐Ÿ˜

Please, could you document on the low level syscalls how the returned error should be handled?

Return the loaded data length or a syscall error

This is definitely not enough documentation for a new developer who is just trying out ckb-std. In my eyes the solution should be pretty similar to how #99 was resolved.

Love & Peace, Phroi

Developers can view SysError directly. Feel free to submit a PR if you believe anything should be added.

Not clear there and, to be honest, it's not even clear on the syscalls RFC... Adding to the things to do: creating two PR.

I'd kindly ask you to leave this issue open until my PR.