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

xCheddar impl #14

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

YellingOilbird
Copy link
Collaborator

add xCheddar lockup token
upgrade cheddar to 4.0.0-pre 7
upgate fn migrate()

add xCheddar lockup token
upgrade cheddar to 4.0.0-pre 7
upgate fn migrate()
Copy link
Contributor

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

partial review

cheddar/README.md Outdated Show resolved Hide resolved
README.md Outdated

## XCheddar token

Token which allows users stake their Cheddar tokens and get some rewards for it. Base model is the same as CRV and veCRV locked tokens model. After 30-days period distribution of rewards starts and it counting from monthly reward parameter. Locked token have a virtual price depends on amount of locked/minted XCheddar tokens
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add a links to CRV and veCRV?

Copy link
Contributor

Choose a reason for hiding this comment

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

ping

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@robert-zaremba yep, what’s required? I update all with ahead commits

Copy link
Contributor

Choose a reason for hiding this comment

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

links to explain CRV and veCRV

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cheddar/src/lib.rs Show resolved Hide resolved
cheddar/src/upgrade.rs Show resolved Hide resolved
@@ -0,0 +1 @@
CONTRACT_NAME=dev-1654515440352-14631167054094
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 we should add neardev to .gitignore

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 do

Copy link
Contributor

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

  • let's don't include the compiled wasm code
  • the math for redeeming xCheddar is not clear (see the comments)
  • timestamp in seconds should be u64, not u32.
  • add docs about msg parameter for ft_transfer_call handler (ft_on_transfer)

@@ -11,10 +11,10 @@ crate-type = ["cdylib", "rlib"]
[dependencies]
serde = { version = "*", features = ["derive"] }
serde_json = "*"
near-sdk = { git = "https://github.com/near/near-sdk-rs", tag="3.1.0" }
near-contract-standards = { git = "https://github.com/near/near-sdk-rs", tag="3.1.0" }
near-sdk = "4.0.0-pre.7"
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
near-sdk = "4.0.0-pre.7"
near-sdk = "4.0.0"

Copy link
Contributor

Choose a reason for hiding this comment

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

v4 is already released, let's don't use -pre

Copy link
Contributor

Choose a reason for hiding this comment

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

we can already use v4.1.0-pre

near-contract-standards = { git = "https://github.com/near/near-sdk-rs", tag="3.1.0" }
near-sdk = "4.0.0-pre.7"
near-sys = "0.1.0"
near-contract-standards = "4.0.0-pre.7"
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
near-contract-standards = "4.0.0-pre.7"
near-contract-standards = "4.0.0"

uint = { version = "0.9.0", default-features = false }

[dev-dependencies]
# near-primitives = { git = "https://github.com/nearprotocol/nearcore.git" }
# near-sdk-sim = { git = "https://github.com/near/near-sdk-rs.git", version="v3.1.0" }
near-sdk-sim = "4.0.0-pre.7"
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
near-sdk-sim = "4.0.0-pre.7"
near-sdk-sim = "4.0.0"

```
rustup target add wasm32-unknown-unknown
RUSTFLAGS='-C link-arg=-s' cargo build --target wasm32-unknown-unknown --release
cp target/wasm32-unknown-unknown/release/cheddar_coin.wasm res/cheddar_coin.wasm
Copy link
Contributor

Choose a reason for hiding this comment

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

we have this in Makefile. Let's use make instruction here.

self.assert_owner();
self.owner_id = owner_id.as_ref().clone();
assert!(
env::is_valid_account_id(owner_id.as_bytes()),
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 we don't need to validate, AccountId should be validated automatically

Copy link
Contributor

Choose a reason for hiding this comment

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

AFIAK, v4 validates account automatically

xcheddar/src/xcheddar.rs Outdated Show resolved Hide resolved
xcheddar/src/xcheddar.rs Outdated Show resolved Hide resolved
}

// return the amount of to be distribute reward this time
pub(crate) fn try_distribute_reward(&self, cur_timestamp_in_sec: u32) -> Balance {
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
pub(crate) fn try_distribute_reward(&self, cur_timestamp_in_sec: u32) -> Balance {
pub(crate) fn compute_distribute_reward(&self, cur_timestamp_in_sec: u32) -> Balance {

self.internal_stake(&sender_id, amount);
PromiseOrValue::Value(U128(0))
} else {
// deposit reward
Copy link
Contributor

Choose a reason for hiding this comment

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

let's use a specific message for making reward desposits. Eg msg: "reward deposit" - otherwise users may run into troubles.
Also update the docs about that.

total_reward += to_yocto("100");

let xcheddar_info0 = view!(xcheddar_contract.contract_metadata()).unwrap_json::<ContractMetadata>();
assert_xcheddar(&xcheddar_info0, to_yocto("100"), 0, 0);
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
assert_xcheddar(&xcheddar_info0, to_yocto("100"), 0, 0);
assert_xcheddar(&xcheddar_info0, total_reward, 0, 0);

@htafolla
Copy link
Contributor

htafolla commented Jul 8, 2022

@robert-zaremba can we finalize and merge, so front-end devs can start work?

YellingOilbird added 2 commits July 21, 2022 01:43
self.assert_owner();
self.owner_id = owner_id.as_ref().clone();
assert!(
env::is_valid_account_id(owner_id.as_bytes()),
Copy link
Contributor

Choose a reason for hiding this comment

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

AFIAK, v4 validates account automatically

ext_ft_resolver::ext(env::current_account_id())
.with_static_gas(GAS_FOR_RESOLVE_TRANSFER)
.ft_resolve_transfer(sender_id, receiver_id, amount.into()),
)
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 want to update to a new API, then let's remove the previous version. Although I would keep the original and only do minimal changes to already deployed token.

}
return used_amount.into();
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why removing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As I correctly remember I do this because of new standards version

@@ -0,0 +1,74 @@
# P3 (NFT versioned) explanation
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we call it p4-farm-nft? We already have p3 farm, and it's working totally differently.

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.

3 participants