runtime-sdk: Add support for (un)delegation receipts #1510

kostko · 2023-09-23T20:17:54Z

This adds support for (un)delegation receipts so contracts can more easily do delegation accounting.

TODO

More tests.

codecov · 2023-09-23T20:19:34Z

Codecov Report

Merging #1510 (1a46fa5) into main (c35d047) will decrease coverage by 0.06%.
The diff coverage is 57.39%.

@@            Coverage Diff             @@
##             main    #1510      +/-   ##
==========================================
- Coverage   60.39%   60.34%   -0.06%     
==========================================
  Files         138      138              
  Lines        9734     9840     +106     
==========================================
+ Hits         5879     5938      +59     
- Misses       3813     3860      +47     
  Partials       42       42

Files Changed	Coverage Δ
runtime-sdk-macros/src/module_derive/mod.rs	`100.00% <ø> (ø)`
...untime-sdk/src/modules/consensus_accounts/types.rs	`60.00% <0.00%> (-40.00%)`	⬇️
runtime-sdk/src/modules/consensus_accounts/mod.rs	`56.04% <35.61%> (-5.37%)`	⬇️
...ime-sdk-macros/src/module_derive/method_handler.rs	`92.80% <100.00%> (+0.87%)`	⬆️
runtime-sdk/src/modules/consensus/mod.rs	`85.71% <100.00%> (+0.25%)`	⬆️
...untime-sdk/src/modules/consensus_accounts/state.rs	`97.76% <100.00%> (+0.41%)`	⬆️

nhynes · 2023-09-25T17:36:55Z

runtime-sdk/src/modules/consensus_accounts/mod.rs


        // Check whether delegate is allowed.
        if params.disable_delegate {
            return Err(Error::Forbidden);
        }
+        // Make sure receipts can only be requested internally (e.g. via subcalls).


why is this required? what's wrong with external receipt storage other than it being weird?

Nothing wrong beyond what you said, just not very useful.

nhynes · 2023-09-25T17:38:33Z

runtime-sdk/src/modules/consensus_accounts/mod.rs

+        let nonce = if store_receipt {
+            body.receipt // Use receipt identifier as the nonce.
+        } else {
+            signer.nonce // Use signer nonce as the nonce.
+        };


Suggested change

let nonce = if store_receipt {

body.receipt // Use receipt identifier as the nonce.

} else {

signer.nonce // Use signer nonce as the nonce.

};

let nonce = store_receipt.then_some(body.receipt).unwrap_or(signer.nonce);

FYI

nhynes · 2023-09-25T17:39:45Z

runtime-sdk/src/modules/consensus_accounts/mod.rs

+        } else {
+            signer.nonce // Use signer nonce as the nonce.
+        };
+        Self::undelegate(ctx, body.from, nonce, to, body.shares, store_receipt)


this is a fair amount of duplicated code. can you split out the common stuff into a function that takes the other functions as input and then does the verification and nonce setup?

Duplication between delegate and undelegate you mean?

nhynes

looks normal to me

nhynes · 2023-09-25T17:40:40Z

runtime-sdk/src/modules/consensus_accounts/mod.rs

@@ -679,14 +772,35 @@ impl<Accounts: modules::accounts::API, Consensus: modules::consensus::API>
        let result: ReclaimEscrowResult = cbor::from_value(result).unwrap();
        let debonding_shares = result.debonding_shares.try_into().unwrap();

-        state::add_undelegation(
+        let receipt = if context.receipt {


Suggested change

let receipt = if context.receipt {

let receipt = context.receipt.then_some(context.nonce).unwrap_or_default();

nhynes · 2023-09-25T17:45:55Z

tests/e2e/contracts/delegation/delegation.sol

+
+        delete(pendingUndelegations[receiptId]);
+    }
+}


strictly speaking, for just testing, I would have passed the cbor directly into the contract and have it proxy the subcall instead. would have been a bit clearer what's going on, too.

Yeah thought about that, I just wanted to write some Solidity to see if the API was viable. Any thoughts about the API, does it make sense?

Could use an existing solidity cbor lib? That would make it clearer to show how it would look like in practice. Unless there's too much work incorporating that into the existing unit tests.

I'm in the process of re-working the CBOR stuff (it was an easy fix to support 64bit ints, but need to do better parsing).

I'm happy with the unit test as a starting point and will be submitting a PR for sapphire-paratime repo when the updated sapphire-dev image is available.

Could use an existing solidity cbor lib? That would make it clearer to show how it would look like in practice. Unless there's too much work incorporating that into the existing unit tests.

Not easily for these E2E tests, we would need to adapt the pipeline a bit.

CedarMist · 2023-09-25T21:50:59Z

There's one thing I need to confirm, to make sure I fully understand.

The same receiptId will be used twice for the UndelegateStart & UndelegateDone receipts.

add_undelegation batches them together on a per-epoch basis, so if I call undelegate twice in a row with two sequential receipt IDs, fetching the two undelegateStart tickets will return the same endReceiptId - which will be the receiptId of the first undelegate call.

kostko · 2023-09-26T04:12:10Z

Yes, note that this is batched per epoch per address the contract is undelegating from.

…ostko/feature/consensus-staking-receipts runtime-sdk: Add support for (un)delegation receipts 9e8c6e9

…/kostko/feature/consensus-staking-receipts runtime-sdk: Add support for (un)delegation receipts 9e8c6e9

peternose · 2023-09-26T10:44:16Z

runtime-sdk/src/modules/consensus_accounts/types.rs

+/// A receipt.
+#[derive(Clone, Debug, Default, cbor::Encode, cbor::Decode)]
+#[cfg_attr(test, derive(PartialEq, Eq))]
+pub struct Receipt {


I prefer splitting this into multiple structs, as it is hard to know which fields are/can be set by different kinds.

Yeah it is just slightly annoying with the current deserializer if you want to keep a flat struct.

kostko force-pushed the kostko/feature/consensus-staking-receipts branch from 7b21437 to 409c1cf Compare September 25, 2023 09:38

kostko added 2 commits September 25, 2023 11:38

runtime-sdk-macros: Add support for internal call handlers

335072a

runtime-sdk: Add support for (un)delegation receipts

1a46fa5

kostko force-pushed the kostko/feature/consensus-staking-receipts branch from 409c1cf to 1a46fa5 Compare September 25, 2023 09:39

kostko marked this pull request as ready for review September 25, 2023 09:39

kostko requested review from pro-wh, ptrus and peternose as code owners September 25, 2023 09:39

nhynes reviewed Sep 25, 2023

View reviewed changes

ptrus approved these changes Sep 26, 2023

View reviewed changes

kostko merged commit 9e8c6e9 into main Sep 26, 2023
25 of 27 checks passed

kostko deleted the kostko/feature/consensus-staking-receipts branch September 26, 2023 09:49

github-actions bot added a commit that referenced this pull request Sep 26, 2023

Deploy js API reference Merge pull request #1510 from oasisprotocol/k…

ea30b9e

…ostko/feature/consensus-staking-receipts runtime-sdk: Add support for (un)delegation receipts 9e8c6e9

github-actions bot added a commit that referenced this pull request Sep 26, 2023

Deploy rust API reference Merge pull request #1510 from oasisprotocol…

fec06fc

…/kostko/feature/consensus-staking-receipts runtime-sdk: Add support for (un)delegation receipts 9e8c6e9

peternose reviewed Sep 26, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

runtime-sdk: Add support for (un)delegation receipts #1510

runtime-sdk: Add support for (un)delegation receipts #1510

kostko commented Sep 23, 2023 •

edited

codecov bot commented Sep 23, 2023 •

edited

nhynes Sep 25, 2023

kostko Sep 25, 2023 •

edited

nhynes Sep 25, 2023

nhynes Sep 25, 2023

kostko Sep 25, 2023

nhynes left a comment

nhynes Sep 25, 2023

nhynes Sep 25, 2023

kostko Sep 25, 2023

ptrus Sep 26, 2023

CedarMist Sep 26, 2023

kostko Sep 26, 2023

CedarMist commented Sep 25, 2023 •

edited

kostko commented Sep 26, 2023

peternose Sep 26, 2023

kostko Sep 26, 2023 •

edited

	let receipt = if context.receipt {
	let receipt = context.receipt.then_some(context.nonce).unwrap_or_default();

runtime-sdk: Add support for (un)delegation receipts #1510

runtime-sdk: Add support for (un)delegation receipts #1510

Conversation

kostko commented Sep 23, 2023 • edited

codecov bot commented Sep 23, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

kostko Sep 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhynes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CedarMist commented Sep 25, 2023 • edited

kostko commented Sep 26, 2023

Choose a reason for hiding this comment

kostko Sep 26, 2023 • edited

Choose a reason for hiding this comment

kostko commented Sep 23, 2023 •

edited

codecov bot commented Sep 23, 2023 •

edited

kostko Sep 25, 2023 •

edited

CedarMist commented Sep 25, 2023 •

edited

kostko Sep 26, 2023 •

edited