-
Notifications
You must be signed in to change notification settings - Fork 204
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
Create ticker on metachain #3363
Conversation
@@ -1816,12 +1824,30 @@ func (d *delegation) computeAndUpdateRewards(callerAddress []byte, delegator *De | |||
|
|||
isOwner := d.isOwner(callerAddress) | |||
|
|||
totalRewards, err := d.computeRewards(delegator.RewardsCheckpoint, isOwner, activeFund.Value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error returned here is not managed below in any way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
d.eei.AddReturnMessage("not enough arguments") | ||
return vmcommon.UserError | ||
} | ||
err := d.eei.UseGas(d.gasCost.MetaChainSystemSCsCost.DelegationOps) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DelegationOps is ok also for liquidStaking or we should define another special cost for it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we also use liquid staking ops cost as well - from liquid staking manager contract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
err := d.eei.UseGas(d.gasCost.MetaChainSystemSCsCost.DelegationOps) | ||
if err != nil { | ||
d.eei.AddReturnMessage(err.Error()) | ||
return vmcommon.UserError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OutOfGas ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
} | ||
|
||
if len(args.Arguments) != 3 { | ||
d.eei.AddReturnMessage("not enough arguments") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
invalid number of arguments ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
return vmcommon.Ok | ||
} | ||
|
||
func (d *delegation) reDelegateRewardsViaLiquidStaking(args *vmcommon.ContractCallInput) vmcommon.ReturnCode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These methods (reDelegateRewardsViaLiquidStaking, unDelegateViaLiquidStaking and returnViaLiquidStaking) will be implemented in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the next PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
return vmcommon.UserError | ||
} | ||
if args.CallValue.Cmp(zero) != 0 { | ||
l.eei.AddReturnMessage("not a payable function") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"function is not payable in eGLD" to keep the same message format used in all the other methods from this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
return vmcommon.UserError | ||
} | ||
if len(args.Arguments) == 0 { | ||
l.eei.AddReturnMessage("not enough arguments") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"invalid number of arguments" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this has concrete check on next PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
return vmcommon.Ok | ||
} | ||
|
||
func (l *liquidStaking) claimRewardsFromDelegatedPosition(args *vmcommon.ContractCallInput) vmcommon.ReturnCode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
claimRewardsViaLiquidStaking is already implemented in delegation.go. This method shouldn't be also implemented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
methods from delegation.go can be called by liquidStaking contract only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
return vmcommon.Ok | ||
} | ||
|
||
func (l *liquidStaking) reDelegateRewardsFromPosition(args *vmcommon.ContractCallInput) vmcommon.ReturnCode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These next methods will be implemented in this PR ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
next PR
message LiquidStakingAttributes { | ||
bytes ContractAddress = 1 [(gogoproto.jsontag) = "ContractAddress"]; | ||
uint32 RewardsCheckpoint = 2 [(gogoproto.jsontag) = "RewardsCheckpoint"]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add empty line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor stuff
value := big.NewInt(0).SetBytes(args.Arguments[1]) | ||
checkPoint := uint32(big.NewInt(0).SetBytes(args.Arguments[2]).Uint64()) | ||
|
||
totalRewards, err := d.computeRewards(checkPoint, false, value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the claim operation via liquid staking will also take all unclaimed rewards (if existing) before creating the liquid position, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good
} | ||
|
||
// ArgsNewLiquidStaking defines the arguments to create the liquid staking smart contract | ||
type ArgsNewLiquidStaking struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
according to our last talk about naming convention, this should be LiquidStakingArgs
return vmcommon.UserError | ||
} | ||
if !l.flagLiquidStaking.IsSet() { | ||
l.eei.AddReturnMessage("liquid staking contract is not enabled") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not backwards compatible if we end up having a transaction towards the liquid staking contract before we have this code up and running.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do have this backward compatible - it is resolved directly in systemVM. line 108-110. It was done for delegation contract first. And contract have activation. They implement CanUseContract()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
const tokenIDKey = "tokenID" | ||
const noncePrefix = "n" | ||
|
||
type liquidStaking struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this sc complete? What it does is that it just have some checks on some operations.
+ do not forget about tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. this is incomplete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
Add ticker on metachain
Add interface to process built in functions easily on metachain