feat(undelegate): use Timestamp instead of Date #9646

0xpatrickdev · 2024-07-03T01:33:59Z

Description

This PR primarily improves unit testing around the .undelegate() flow and was motivated by a failure (inability to decode completionTime) discovered in E2E testing.

Add tests for cosmos-orchestration-account staking flows using mocked IBC messages
Use Timestamp (from google/protobuf/timestamp.proto) instead of Date for @agoric/cosmic-proto codegen for better endo compatibility
Update undelegate() to use Timestamp for wakeAt() calculation

Testing Considerations

This PR includes additional tests for cosmos-orchestration-account flows.

Upgrade Considerations

This is a breaking change for @agoric/cosmic-proto, but not for the agoric namespace.

0xpatrickdev · 2024-07-03T01:42:45Z

packages/orchestration/src/exos/local-orchestration-account.js

@@ -87,7 +88,9 @@ export const prepareLocalOrchestrationAccountKit = (
    {
      holder: HolderI,
      undelegateWatcher: M.interface('undelegateWatcher', {
-        onFulfilled: M.call([M.splitRecord({ completionTime: M.string() })])
+        onFulfilled: M.call([
+          M.splitRecord({ completionTime: TimestampProtoShape }),


Using splitRecord here to make this forward compatible (cosmos v0.50 adds amount: Coin to the response: https://buf.build/cosmos/cosmos-sdk/docs/main:cosmos.staking.v1beta1#cosmos.staking.v1beta1.MsgUndelegateResponse)

Are there other places we might need to consider this approach? Should we be guarding watcher's more loosely?

I guess the local-orch-account is less likely to get an unexpected upgrade to v0.50, more the cosmos-orch-account. We do not have something similar in those interface guards since the value is still a base64 byte string at that point.

Good question tho. Thanks for raising and resolving.

cloudflare-workers-and-pages · 2024-07-03T02:20:59Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`3d4ab30`
Status:	✅ Deploy successful!
Preview URL:	https://a95a73a6.agoric-sdk.pages.dev
Branch Preview URL:	https://pc-cosmos-orch-account-tests.agoric-sdk.pages.dev

View logs

0xpatrickdev · 2024-07-03T13:53:56Z

packages/cosmic-proto/test/helpers.test-d.ts

  const responseJson: JsonSafe<typeof response> = null as any;
-  expectType<string>(responseJson.completionTime);
+  // FIXME: should be a string. UNTIL: github.com/cosmology-tech/telescope/pull/632
+  expectType<bigint>(responseJson.completionTime.seconds);


FYI. didn't seem worth checking in a patch, but happy to make one

turadg

Thanks for the clearly described separate commits.

Only minor issues. Approving assuming they'll be addressed before merge.

turadg · 2024-07-03T14:00:15Z

packages/orchestration/test/examples/stake-ica.contract.test.ts

+  });
+  t.is(delegation, undefined, 'delegation returns void');
+
+  // TODO, fixme!


I was reviewing by commit so I drafted a comment asking when, then I saw the fix in a subsequent PR.

I support this style of queuing up work for other commits in the PR. Consider being more specific for the reader, e.g. FIXME in this PR

Acknowledged for the future

turadg · 2024-07-03T14:00:57Z

packages/orchestration/test/examples/stake-ica.contract.test.ts

+    },
+  );
+
+  const redelgation = await E(account).redelegate(


turadg · 2024-07-03T14:01:50Z

packages/orchestration/test/examples/stake-ica.contract.test.ts

+      ...validatorAddr,
+      address: 'cosmosvaloper2test',


our composite "address" object reads so weirdly. I hope we can come up with a better name.

cc @mitdralla

turadg · 2024-07-03T14:11:33Z

packages/orchestration/src/exos/local-orchestration-account.js

@@ -87,7 +88,9 @@ export const prepareLocalOrchestrationAccountKit = (
    {
      holder: HolderI,
      undelegateWatcher: M.interface('undelegateWatcher', {
-        onFulfilled: M.call([M.splitRecord({ completionTime: M.string() })])
+        onFulfilled: M.call([
+          M.splitRecord({ completionTime: TimestampProtoShape }),


Good question tho. Thanks for raising and resolving.

turadg · 2024-07-03T14:13:20Z

packages/orchestration/src/typeGuards.js

+ * for google/protobuf/timestamp.proto, not to be confused with TimestampShape
+ * from `@agoric/time`


excellent comment

turadg · 2024-07-03T14:13:54Z

packages/orchestration/src/utils/time.js

- * @param {Date} date
- * @returns {bigint}
- */
-export const dateInSeconds = date => BigInt(Math.floor(date.getTime() / 1000));


reduction in code affirms your idea to switch to Timestamp

turadg · 2024-07-03T14:19:20Z

packages/orchestration/test/ibc-mocks.ts

+/**
+ * returns Timestamp record 5 seconds from unix epoch.
+ */
+const getCompletionTime = (): Timestamp => {
+  return {
+    seconds: UNBOND_PERIOD_SECONDS,
+    nanos: 0,
+  };
+};


it's only five because of UNBOND_PERIOD_SECONDS. if someone changes that they won't know to change this comment.

the 0+ is thoughtful, but I don't think the "since unix epoch" is relevant to the test

please simplify and make more explicit what it gets

Suggested change

/**

* returns Timestamp record 5 seconds from unix epoch.

*/

const getCompletionTime = (): Timestamp => {

return {

seconds: UNBOND_PERIOD_SECONDS,

nanos: 0,

};

};

const getUnbondingTime = () => new Date(Number(UNBOND_PERIOD_SECONDS * MILLISECONDS_PER_SECOND));

Simplified but kept the return type a Timestamp

oh good. I made the suggestion on an earlier commit and when I pulled it to the PR level I forgot to adjust the return type

dckc · 2024-07-03T13:50:22Z

packages/orchestration/src/cosmos-api.ts

@@ -212,6 +212,13 @@ export type LiquidStakingMethods = {
  liquidStake: (amount: AmountArg) => Promise<void>;
 };

+export type LocalAccountMethods = {
+  /** deposit payment from zoe to the account */


Suggested change

/** deposit payment from zoe to the account */

/** deposit payment (from zoe, for example) to the account */

Zoe isn't the only source of payments.

dckc · 2024-07-03T14:18:54Z

packages/orchestration/src/examples/sendAnywhere.contract.js

@@ -68,6 +68,8 @@ const sendItFn = async (
  const { chainId } = info;
  assert(typeof chainId === 'string', 'bad chainId');
  const { [kw]: pmtP } = await withdrawFromSeat(zcf, seat, give);
+  // #9212 types for chain account helpers
+  // @ts-expect-error LCA should have .deposit() method


dckc · 2024-07-03T14:52:40Z

packages/orchestration/src/orchestration-api.ts

-   * deposit payment from zoe to the account. For remote accounts,
-   * an IBC Transfer will be executed to transfer funds there.
-   */
-  deposit: (payment: Payment<'nat'>) => Promise<void>;


cc @dtribble - change to orchestration-api.ts

cc @mitdralla too

Similar story to: #9591 (comment)

dckc

very nicely done!

dckc · 2024-07-03T15:02:38Z

packages/orchestration/test/staking-ops.test.ts

@@ -92,7 +99,7 @@ const makeScenario = () => {

      '/cosmos.staking.v1beta1.MsgBeginRedelegate': _m => {
        const response = MsgBeginRedelegateResponse.fromPartial({
-          completionTime: new Date('2025-12-17T03:24:00Z'),
+          completionTime: dateToTimestamp(new Date('2025-12-17T03:24:00Z')),


here's hoping we manage to completely replace the mock stuff here with using ibc-mocks

Agree. It seems there's parity here with stake-ica.test.ts minus the wallet offers.

Will +1 that we should be able to use a macro and test LocalOrchestrationAccount in the same flow. Currently there are two small blockers to that:

LOA delegate and undelegate should accept a ChainAddress obj instead of a string

LOA undelegate should accept an array of delegations

dckc · 2024-07-03T15:07:48Z

packages/orchestration/src/exos/local-orchestration-account.js

-              // TODO clean up date handling once we have real data
-              dateInSeconds(new Date(completionTime)) + maxClockSkew,
+              // ignore nanoseconds and just use seconds from Timestamp
+              BigInt(completionTime.seconds) + maxClockSkew,


👏 good to see the TODOs getting to-done.

dckc · 2024-07-03T15:08:39Z

packages/orchestration/test/examples/stake-ica.contract.test.ts

@@ -155,17 +159,23 @@ test('delegate, undelegate, redelegate, withdrawReward', async t => {
  });
  t.is(delegation, undefined, 'delegation returns void');

-  // TODO, fixme!


dckc · 2024-07-03T15:11:01Z

packages/orchestration/test/exos/local-orchestration-account-kit.test.ts

+  t.is(
+    await undelegateP,
+    undefined,
+    'undelegate returns void after completion_time',


you can test that it actually waited that long by racing with timer.wakeAt(expectedFullfillment - 1n)

Updated to include, ty. Also realized I don't need to provide a brand to the zone manual timer.

dckc

oops; some comments pending from another window

- removes .deposit() from CosmosOrchestrationAccount as #9193 stipulates this will be done via an LCA - adds LocalAccountMethods type, although unused, for documenatation purposes. and, eventually, to support #9212

- test cosmos-orchestration-account.js via stakeIca.contract.js using mocked messages

- updates typingsFormat.timestamp to 'timestamp' instead of 'date' for better SES compatability - this Timestamp is from google/protobuf/timestamp.proto and looks like { seconds: bigint; nanos: number }

- removes dateInSeconds, as cosmic-proto now returns { seconds: bigint; nanos: number } - nanoseconds are ignored for the wakeAt() calculation, as these seem immaterial - adds undelegate() tests for LOA and COA that advance timer service to verify behavior - updates LOA to return undefined instead of a TimestampRecord on .delegate()

0xpatrickdev commented Jul 3, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/cosmos-orch-account-tests branch from 608499d to f77dc70 Compare July 3, 2024 02:20

0xpatrickdev force-pushed the pc/cosmos-orch-account-tests branch 4 times, most recently from ba3e74e to 433ded2 Compare July 3, 2024 13:42

0xpatrickdev marked this pull request as ready for review July 3, 2024 13:46

0xpatrickdev requested review from turadg and dckc July 3, 2024 13:46

0xpatrickdev commented Jul 3, 2024

View reviewed changes

turadg approved these changes Jul 3, 2024

View reviewed changes

dckc requested a review from dtribble July 3, 2024 14:52

dckc reviewed Jul 3, 2024

View reviewed changes

dckc approved these changes Jul 3, 2024

View reviewed changes

dckc requested a review from mitdralla July 3, 2024 15:12

dckc reviewed Jul 3, 2024

View reviewed changes

turadg added the force:integration Force integration tests to run on PR label Jul 3, 2024

turadg mentioned this pull request Jul 3, 2024

feat(cosmic-proto): add osmosis protos and refresh #9649

Merged

0xpatrickdev force-pushed the pc/cosmos-orch-account-tests branch from 433ded2 to c6ca8a8 Compare July 3, 2024 17:46

0xpatrickdev added 4 commits July 3, 2024 14:44

chore: cosmos-orch-account does not have .deposit()

061accd

- removes .deposit() from CosmosOrchestrationAccount as #9193 stipulates this will be done via an LCA - adds LocalAccountMethods type, although unused, for documenatation purposes. and, eventually, to support #9212

test: cosmos-orchestration-account flows

b426fa9

- test cosmos-orchestration-account.js via stakeIca.contract.js using mocked messages

feat!: use Timestamp instead of Date

0ac64b7

- updates typingsFormat.timestamp to 'timestamp' instead of 'date' for better SES compatability - this Timestamp is from google/protobuf/timestamp.proto and looks like { seconds: bigint; nanos: number }

0xpatrickdev force-pushed the pc/cosmos-orch-account-tests branch from c6ca8a8 to 3d4ab30 Compare July 3, 2024 18:44

0xpatrickdev added the automerge:rebase Automatically rebase updates, then merge label Jul 3, 2024

mergify bot merged commit c2222f8 into master Jul 3, 2024
80 of 85 checks passed

mergify bot deleted the pc/cosmos-orch-account-tests branch July 3, 2024 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(undelegate): use Timestamp instead of Date #9646

feat(undelegate): use Timestamp instead of Date #9646

0xpatrickdev commented Jul 3, 2024 •

edited

Loading

0xpatrickdev Jul 3, 2024

0xpatrickdev Jul 3, 2024

turadg Jul 3, 2024

cloudflare-workers-and-pages bot commented Jul 3, 2024 •

edited

Loading

0xpatrickdev Jul 3, 2024

turadg left a comment

turadg Jul 3, 2024

0xpatrickdev Jul 3, 2024

turadg Jul 3, 2024

turadg Jul 3, 2024

dckc Jul 3, 2024

turadg Jul 3, 2024

turadg Jul 3, 2024

turadg Jul 3, 2024

turadg Jul 3, 2024

0xpatrickdev Jul 3, 2024 •

edited

Loading

turadg Jul 3, 2024

dckc Jul 3, 2024

dckc Jul 3, 2024

dckc Jul 3, 2024

dckc Jul 3, 2024

0xpatrickdev Jul 3, 2024

dtribble Jul 3, 2024

dckc left a comment

dckc Jul 3, 2024

0xpatrickdev Jul 3, 2024 •

edited

Loading

dckc Jul 3, 2024

dckc Jul 3, 2024

dckc Jul 3, 2024

0xpatrickdev Jul 3, 2024

dckc left a comment

		* for google/protobuf/timestamp.proto, not to be confused with TimestampShape
		* from `@agoric/time`

	/** deposit payment from zoe to the account */
	/** deposit payment (from zoe, for example) to the account */

feat(undelegate): use Timestamp instead of Date #9646

feat(undelegate): use Timestamp instead of Date #9646

Conversation

0xpatrickdev commented Jul 3, 2024 • edited Loading

Description

Testing Considerations

Upgrade Considerations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Jul 3, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

Choose a reason for hiding this comment

turadg 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xpatrickdev Jul 3, 2024 • edited Loading

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

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xpatrickdev Jul 3, 2024 • edited Loading

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

dckc left a comment

Choose a reason for hiding this comment

0xpatrickdev commented Jul 3, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Jul 3, 2024 •

edited

Loading

0xpatrickdev Jul 3, 2024 •

edited

Loading

0xpatrickdev Jul 3, 2024 •

edited

Loading