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

Min max delegation amounts #4569

Merged
merged 51 commits into from
Jun 10, 2024

Conversation

moubctez
Copy link
Collaborator

@moubctez moubctez commented Feb 28, 2024

Implements casper-network/ceps#90

Resolves #4860 [Issue created retrospectively]

@wojcik91
Copy link
Collaborator

@mpapierski ping

@wojcik91
Copy link
Collaborator

@mpapierski We added the forced undelegation process as discussed last week in f1a1400

@moubctez moubctez marked this pull request as ready for review March 20, 2024 13:32
Copy link
Collaborator

@mpapierski mpapierski left a comment

Choose a reason for hiding this comment

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

Looking good code-wise. It'd be a good idea to measure how many delegators will be unbonded when 500 CSPR minimum delegation is enforced. Queueing unbonding requests for too many delegators at once may affect the network performance for some time until the queue is cleared.

smart_contracts/contract/src/contract_api/runtime.rs Outdated Show resolved Hide resolved
Copy link
Contributor

Timed out.

@mpapierski
Copy link
Collaborator

bors r+

Copy link
Contributor

Canceled.

@mpapierski
Copy link
Collaborator

bors r+

casperlabs-bors-ng bot added a commit that referenced this pull request Jun 6, 2024
4569: Min max delegation amounts r=mpapierski a=moubctez

Implements casper-network/ceps#90

Co-authored-by: Adam Ciarciński <aciarcinski@teonite.com>
Co-authored-by: Maciek <tapczan666+github@gmail.com>
Co-authored-by: Maciej Wójcik <wojcik91@gmail.com>
Copy link
Contributor

Build failed:

Copy link
Contributor

🔒 Permission denied

Existing reviewers: click here to make moubctez a reviewer

@mpapierski
Copy link
Collaborator

bors r+

casperlabs-bors-ng bot added a commit that referenced this pull request Jun 6, 2024
4569: Min max delegation amounts r=mpapierski a=moubctez

Implements casper-network/ceps#90

Co-authored-by: Adam Ciarciński <aciarcinski@teonite.com>
Co-authored-by: Maciek <tapczan666+github@gmail.com>
Co-authored-by: Maciej Wójcik <wojcik91@gmail.com>
Copy link
Contributor

Timed out.

@mpapierski
Copy link
Collaborator

bors r+

casperlabs-bors-ng bot added a commit that referenced this pull request Jun 7, 2024
4569: Min max delegation amounts r=mpapierski a=moubctez

Implements casper-network/ceps#90

Co-authored-by: Adam Ciarciński <aciarcinski@teonite.com>
Co-authored-by: Maciek <tapczan666+github@gmail.com>
Co-authored-by: Maciej Wójcik <wojcik91@gmail.com>
Copy link
Contributor

Timed out.

@sacherjj sacherjj merged commit 0685d39 into casper-network:feat-2.0 Jun 10, 2024
2 checks passed
// Avoids allocation with 0 bytes and a call to get_named_arg
Vec::new()
};
bytesrepr::deserialize(arg_bytes).unwrap_or_revert_with(ApiError::InvalidArgument)
Copy link

Choose a reason for hiding this comment

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

@moubctez Hi, something is strange here I don't get this function to work as I understood its purpose, not sure if this is on my contract but I get reverted with InvalidArgument passing value, according to the comment (not an Option though)

I would invite to test this function again because according to my test get_named_arg and try_get_named_arg do the exact same thing with Option for get_named_arg, while try_get_named_arg should handle absence of argument (not None) as per code comment.
test calling

        let _ = args.insert("TEST1", None::<u64>);
        let _ = args.insert("TEST2", None::<u64>);
        let _ = args.insert("TEST3", 111_u64);
        let _ = args.insert("TEST4", Some(111_u64));
        let _ = args.insert("TEST5", 111_u64);

contract

    let test1: Option<u64> = get_named_arg("TEST1");
    let test2: Option<u64> = try_get_named_arg("TEST2");

    let test3: u64 = get_named_arg("TEST3");
    let test4: Option<u64> = try_get_named_arg("TEST4");
    
    runtime::print(&format!("{:?}", test1));
    runtime::print(&format!("{:?}", test2));
    runtime::print(&format!("{:?}", test3));
    runtime::print(&format!("{:?}", test4));

    let test5: Option<u64> = try_get_named_arg("TEST5");
    runtime::print(&format!("will revert, never printed{:?}", test5));
        

Regards

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've re-examined the code and as far as I can recall this is a leftover from my attempt to handle optional arguments that is not used anywhere and can be removed.

Copy link

Choose a reason for hiding this comment

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

Hi, I believe try_get_named_arg intention was to handle no-value/value but now it deals with no-value/Some(value) which is not really expected on usage. get_named_arg deals with None/Some(value) which requires the parameter to be given has Option in any case.

There is however a use case thus for an get_optional_named_arg not expecting an Option but I think right now it's indeed better to remove this function as confusing.

#4786

gRoussac added a commit to gRoussac/casper-node that referenced this pull request Jul 2, 2024
As not handling no-value / value but no-value/Some(value)

unused yet
casperlabs-bors-ng bot added a commit that referenced this pull request Jul 18, 2024
4786: Remove revert on deser in try_get_named_arg introduced in #4569 r=darthsiroftardis a=gRoussac

Remove revert in try_get_named_arg introduced in #4569

#4569 (comment)

method unused yet in contracts


Co-authored-by: gRoussac <gRoussac@users.noreply.github.com>
@devendran-m devendran-m added the rc-3 Release Candidate 3 label Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rc-3 Release Candidate 3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants