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

Document SysError::LengthNotEnough intended usage #100

Open
phroi opened this issue Aug 12, 2024 · 9 comments
Open

Document SysError::LengthNotEnough intended usage #100

phroi opened this issue Aug 12, 2024 · 9 comments

Comments

@phroi
Copy link

phroi commented Aug 12, 2024

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

@XuJiandong
Copy link
Collaborator

The document is here:

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

@phroi
Copy link
Author

phroi commented Sep 4, 2024

@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),
        }
    }
}

@phroi
Copy link
Author

phroi commented Sep 4, 2024

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

@XuJiandong
Copy link
Collaborator

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

@phroi
Copy link
Author

phroi commented Sep 4, 2024

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

@XuJiandong
Copy link
Collaborator

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.

@phroi
Copy link
Author

phroi commented Sep 5, 2024

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

@XuJiandong
Copy link
Collaborator

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

@phroi
Copy link
Author

phroi commented Sep 5, 2024

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.

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

No branches or pull requests

2 participants