feat(node): integration rewards, review comments.#1205
feat(node): integration rewards, review comments.#1205raulk merged 35 commits intointegration/rewardsfrom
Conversation
| } | ||
|
|
||
| /// The proofs to batch claim validator rewards in a specific subnet | ||
| /// REVIEW(raulk): Delete this type. Make the method just take the subnet ID and the list of claim proofs. |
There was a problem hiding this comment.
@raulk I think this type is introduced to handle multiple subnet ids at once. With this structure, might be easier to manage types. Or this requirement is not needed?
There was a problem hiding this comment.
So there's two levels of batching:
- Claim rewards corresponding to multiple checkpoint heights for a single subnet at once.
- Claim rewards from multiple subnets, over multiple checkpoint heights for each subnet, at once.
I would only support (1) and not (2) as:
- It limits attack surface by constraining interactions to a single subnet within a transaction.
- It sounds like premature optimization to me, and once introduced it will be impossible to remove even if nobody uses it.
My suggestion is that we descope (2) and limit ourselves to (1). With that in mind, we can kill the extra type and simply have three arguments:
- subnet ID
- array of checkpoint heights
- array of claims
cryptoAtwill
left a comment
cryptoAtwill
left a comment
There was a problem hiding this comment.
@raulk I have push the changes to the contract. Please help take a quick look as there are some slight adjustments to fit the overall flow. Once this is ok, fendermint is very easy to change.
| /// The proof required for validators to claim rewards | ||
| struct ValidatorClaimProof { | ||
| ValidatorSummary summary; | ||
| uint64 checkpointHeight; |
There was a problem hiding this comment.
@raulk Add in checkpoint height to identify which commitment to use
There was a problem hiding this comment.
I think this should be an unwrapped argument. It is not part of the proof per se, but rather an extra argument we need to pass to the method to identify the height against we're presenting the proof. For the batch claim, we should take an array of heights and an array of proofs.
There was a problem hiding this comment.
See related design comment: #1205 (comment).
cryptoAtwill
changed the title
- rename and retype to attempt to stabilize the data model. - introduced the notion of activity bundles, validator data, compressed summary, full summary. - remove wrapped types for arguments to reduce type complexity. - constrain batch claims to a single origin subnet to simplify solution. still doesn't compile / need help!
…yard/ipc into raulk/rewards-review
Co-authored-by: cryptoAtwill <willes.lau@protocol.ai> Co-authored-by: raulk <raul.kripalani@gmail.com>
2 participants
Calibration testing:
SubnetRegistryDiamond deployed at 0xbCC8CAbbf2f33D5665B176692CDc6B1135bAE05E
GatewayDiamond deployed at 0x689D7fAD1a77d9aDc7d2C724c366C8EF8D919184