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

feat: complete UndelegateAndTransfer and DepositAndDelegate flows #10045

Merged
merged 9 commits into from
Sep 10, 2024

Conversation

0xpatrickdev
Copy link
Member

@0xpatrickdev 0xpatrickdev commented Sep 7, 2024

closes: #10046

Description

  • use AnyNatAmountShape for AmountArg in @agoric/orchestration
  • Update undelegate to take AmountArg and ChainAddress
  • Update undelegate to accept and bondDenom
  • Finish UnelegateAndTransfer and DepositAndDelegate flows in staking-combinations contract

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

n/a

Testing Considerations

Updates existing tests and includes tests for new functionality

Upgrade Considerations

n/a

Copy link

cloudflare-workers-and-pages bot commented Sep 7, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 2a50005
Status: ✅  Deploy successful!
Preview URL: https://a8bbd1e7.agoric-sdk.pages.dev
Branch Preview URL: https://pc-staking-combinations.agoric-sdk.pages.dev

View logs

Base automatically changed from ta/simplify-staking-combinations to master September 7, 2024 18:58
Copy link

github-actions bot commented Sep 7, 2024

Base branch is changed to master. Please re-run the integration tests by adding 'force:integration' label.

@0xpatrickdev 0xpatrickdev force-pushed the pc/staking-combinations branch 2 times, most recently from 928bdfc to 8120eb2 Compare September 7, 2024 20:21
@0xpatrickdev 0xpatrickdev changed the title feat: undelegate takes AmountArg and ChainAddress feat: complete UndelegateAndTransfer and DepositAndDelegate flows Sep 7, 2024
@0xpatrickdev 0xpatrickdev marked this pull request as ready for review September 7, 2024 20:40
Comment on lines 71 to 72
: T[K] extends (...args: any[]) => infer R
? (...args: Parameters<T[K]>) => R
Copy link
Member

Choose a reason for hiding this comment

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

Isn' this just:

Suggested change
: T[K] extends (...args: any[]) => infer R
? (...args: Parameters<T[K]>) => R
: T[K] extends (...args: any[]) => infer R
? T[K]

I don't think that's quite right though. What failure did you run into? Please add the cases to async-flow/test/types.test-d.ts so we can avoid regressions in these types.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, this is simpler and works. The error I was seeing was: Type 'GuestInterface<() => HostInterface<ChainAddress>>' has no call signatures.

I added a type test in f10bad5.

I was expecting to be able to do:

@@ -1,5 +1,6 @@
 import { expectType } from 'tsd';
 import type { Vow, VowTools } from '@agoric/vow';
+import type { Guarded } from '@endo/exo';
 import type {
   HostOf,
   GuestOf,
@@ -30,6 +31,7 @@ type ExoAPIBase = {
   getValue: () => number;
   setValue: (value: number) => void;
   getCopyData: () => Record<string, number>[];
+  getRemote: () => Guarded<{ doFoo: () => void }>;
 };
 type ExoGuestAPI = ExoAPIBase & {
   getValueAsync: () => Promise<number>;

But see:

Type 'HostInterface<Guarded<{ doFoo: () => void; }>>' is missing the following properties from type 'RemotableBrand<{}, { doFoo: () => void; } & GetInterfaceGuard<{ doFoo: () => void; }>>': L, R

Is Guarded the correct way to type a durable remotable?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, Guarded should:

export type Farable<M extends Methods> = M &
  RemotableBrand<{}, M> &
  RemotableObject;
export type Guarded<M extends Methods> = Farable<M & GetInterfaceGuard<M>>;

but RemotableBrand expects these private properties,

export declare class RemotableBrand<Local, Remote> {
  /** The local properties of the object. */
  private L: Local;

  /** The type of all the remotely-callable functions. */
  private R: Remote;
}

I suppose we need to fix RemotableBrand so it doesn't appear to have L and R as real properties. WDYT @michaelfig ?

Meanwhile I think it's okay to cast or expect error with an explanation

Copy link
Member

Choose a reason for hiding this comment

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

Please don't change RemotableBrand... it's carefully tuned to give proper errors when you pass it an object that wasn't created correctly as a Remotable. Consider it an internal type that makes E work with things like Exo-objects, Remotable, Far, and durables, not for use outside that layer.

To me, it seems that HostInterface is at fault; it needs to explicitly handle any RemotableBrand that was implied by its template parameter.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't this just:

Not the same, but I'm not sure what was intended.

The more complex version returned the type of just the function itself (such as the type of a simple wrapper function), but the "simpler" version preserved the type of the function including all the subproperties that might be hanging off of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Meanwhile I think it's okay to cast or expect error with an explanation

I just left a TODO for now

but I'm not sure what was intended

The changes were to support: getChainAddress: () => ChainAddress

@@ -17,7 +17,7 @@ See [`src/examples`](src/examples)
| [send-anywhere](/packages/orchestration/src/examples/send-anywhere.contract.js) | Ready 🟢 | Allows sending payments (tokens) over IBC to another chain. | - `LocalOrchestrationAccoun`t<br>- `Vtransfer` (IBC Hooks) |
| [stakeBld](/packages/orchestration/src/examples/stakeBld.contract.js) | Ready 🟢 | Returns a `LocalOrchestrationAccount` that can perform staking actions. | - `LocalOrchestrationAccount` | Ready 🟢 |
| [stakeIca](/packages/orchestration/src/examples/stakeIca.contract.js) | Ready 🟢 | Returns a `CosmosOrchestrationAccount` that can perform staking actions. | - `CosmosOrchestrationAccount` | Ready 🟢 |
| [staking-combinations](/packages/orchestration/src/examples/staking-combinations.contract.js) | Under Construction 🚧 | Combines actions into a single offer flow and demonstrates writing continuing offers. | - `CosmosOrchestrationAccount`<br>- `CombineInvitationMakers` <br>- Continuing Offers |
| [staking-combinations](/packages/orchestration/src/examples/staking-combinations.contract.js) | Ready 🟢 | Combines actions into a single offer flow and demonstrates writing continuing offers. | - `CosmosOrchestrationAccount`<br>- `CombineInvitationMakers` <br>- Continuing Offers |
Copy link
Member

Choose a reason for hiding this comment

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

🎉

'Deposit and delegate',
undefined,
{
give: {
Stake: AmountShape,
},
want: {},
// user cannot exit their seat; contract must exit it.
Copy link
Member

Choose a reason for hiding this comment

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

ty

@@ -37,7 +37,7 @@ export const prepareChainHubAdmin = (zone, chainHub) => {
* @param {CosmosChainInfo} chainInfo
* @param {IBCConnectionInfo} connectionInfo
*/
async initChain(chainName, chainInfo, connectionInfo) {
async registerChain(chainName, chainInfo, connectionInfo) {
Copy link
Member

Choose a reason for hiding this comment

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

better name. I forget why we had init before. Maybe thinking it would be only allowed once? but clearly what it's doing is registering

packages/orchestration/src/typeGuards.js Outdated Show resolved Hide resolved
@0xpatrickdev 0xpatrickdev force-pushed the pc/staking-combinations branch 4 times, most recently from cab6733 to 2952b9a Compare September 10, 2024 16:21
@0xpatrickdev 0xpatrickdev added the force:integration Force integration tests to run on PR label Sep 10, 2024
packages/async-flow/test/types.test-d.ts Outdated Show resolved Hide resolved
@0xpatrickdev 0xpatrickdev force-pushed the pc/staking-combinations branch 2 times, most recently from 09f7511 to 0ac0e68 Compare September 10, 2024 18:58
@0xpatrickdev 0xpatrickdev added the automerge:rebase Automatically rebase updates, then merge label Sep 10, 2024
@0xpatrickdev 0xpatrickdev force-pushed the pc/staking-combinations branch 2 times, most recently from 21ee795 to 0d8638b Compare September 10, 2024 21:15
- updates `MAKE_ACCOUNT_AND_QUERY_BALANCE_TIMEOUT` after observing:
  - incoming offer at "2024-09-10T17:40:50.076Z" and offer result at "2024-09-10T17:42:16.158Z"
  - in https://github.com/Agoric/agoric-sdk/actions/runs/10797610413/job/29949245444?pr=10053#step:12:3673
@mergify mergify bot merged commit 607ed82 into master Sep 10, 2024
80 checks passed
@mergify mergify bot deleted the pc/staking-combinations branch September 10, 2024 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

staking-combinations.contract.js is incomplete
3 participants