feat: `localOrchAccount.getBalance` (non-vbank assets) and `localOrchAccount.getBalances` #10029

0xpatrickdev · 2024-09-04T19:21:26Z

closes: #9610

Description

updates LocalOrchestrationAccount.getBalance() to support non-vbank assets (using localchain.query() to ask cosmos bank)
implements LocalOrchestrationAccount.getBalances()
BREAKING: renames ChainHub.lookupDenom and ChainHub.lookupAsset to getDenom and getAsset
BREAKING: ChainHub.getAsset returns undefined instead of throwing when lookup fails

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

Updates pertinent jsodc and exos/README.md

Testing Considerations

Includes unit and e2e tests

Upgrade Considerations

n/a, unreleased code

cloudflare-workers-and-pages · 2024-09-04T19:24:03Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`687adf1`
Status:	✅ Deploy successful!
Preview URL:	https://6949c637.agoric-sdk.pages.dev
Branch Preview URL:	https://pc-local-orch-query-balance.agoric-sdk.pages.dev

View logs

0xpatrickdev · 2024-09-04T19:37:36Z

packages/orchestration/src/utils/amounts.js

+  } catch (_) {
+    return undefined;
+  }
+};


In the course of adding these, I considered refactoring these helpers to something like:

type DenomAmountHelper = { coerceDenom: (denomArg: DenomArg) => Denom; coerceDenomAmount: (amount: DenomAmount | Amount<'nat'>) => DenomAmount; coerceCoin: (amount: AmountArg | Amount<'nat'>) => Coin; getAssetSafe: (denom: Denom) => DenomDetail | undefined; getDenomSafe: (brand: Brand<'nat'>) => Denom | undefined; }; type makeDenomAmountHelper = (chainHub: ChainHub) => DenomAmountHelper;

so we can use it more conveniently. However, that might beg the question as to why these helpers aren't in ChainHub.

Anyone have thoughts/opinions here? cc @turadg @dckc

I think we should drop the Safe methods. I don't have a strong opinion on whether to curry the chainhub into the coercers but I lean towards the current factoring as it's more flexible.

multichain-testing/test/account-balance-queries.test.ts

turadg · 2024-09-04T19:56:27Z

packages/orchestration/src/exos/chain-hub.js

@@ -395,21 +395,20 @@ export const makeChainHub = (agoricNames, vowTools) => {
     *
     * @param {Denom} denom
     * @returns {DenomDetail}
+     * @throws {Error} if denom not found (unregistered)


there's no case in which undefined is a valid return. let's just return undefined when none is defined.

turadg · 2024-09-04T19:59:54Z

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

+            if (!denom) {
+              throw Fail`No denomination for brand: ${denomArg}`;
+            }


this should still be after denom is defined for type narrowing.

and then the if (brand && denom) can be simplified to,

if (brand) { } else { }

turadg · 2024-09-04T20:01:17Z

packages/orchestration/src/utils/amounts.js

-  if (!denom) {
-    throw makeError(`No denomination for brand ${denomArg}`);
-  }


turadg · 2024-09-04T20:01:48Z

packages/orchestration/src/utils/amounts.js

+ * @param {Denom} denom
+ * @returns {DenomDetail | undefined}
+ */
+export const getAssetSafe = (chainHub, denom) => {


let's obviate

turadg · 2024-09-04T20:02:25Z

packages/orchestration/src/utils/amounts.js

+  } catch (_) {
+    return undefined;
+  }
+};


I think we should drop the Safe methods. I don't have a strong opinion on whether to curry the chainhub into the coercers but I lean towards the current factoring as it's more flexible.

turadg · 2024-09-04T20:04:35Z

packages/orchestration/test/supports.ts

@@ -156,7 +156,7 @@ export const commonSetup = async (t: ExecutionContext<any>) => {
   */
  const registerAgoricBld = () => {
    try {
-      chainHub.lookupAsset('ubld');
+      chainHub.getAsset('ubld');


making getAsset stop throwing will need to be handled here

github-actions · 2024-09-04T21:16:09Z

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

turadg · 2024-09-04T21:29:02Z

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

-            throw Fail`No denom for ${denomArg}`;
-          }
+            if (!denom) {
+              throw Fail`No denomination for brand: ${denomArg}`;


"denom" was more accurate https://tutorials.cosmos.network/tutorials/6-ibc-dev/

turadg · 2024-09-04T21:31:15Z

packages/orchestration/src/exos/orchestrator.js

-          const { chainName, baseName, baseDenom, brand } =
-            chainHub.lookupAsset(denom);
+          const denomDetail = chainHub.getAsset(denom);
+          if (!denomDetail) throw Fail`No denomination detail for ${q(denom)}`;


again, denom

turadg · 2024-09-04T21:32:35Z

packages/orchestration/src/utils/amounts.js

@@ -10,12 +10,14 @@ import { makeError } from '@endo/errors';
 * @param {ChainHub} chainHub
 * @param {DenomArg} denomArg
 * @returns {Denom}
+ * @throws {Error} if Brand is provided and ChainHub does't contain Brand:Denom


turadg · 2024-09-04T21:33:26Z

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

+
+  t.deepEqual(
+    await VE(account).getBalances(),
+    // 'fake bridge is mocked to return 10 ubld and 10 uist balance'


could you import these from that module?

turadg · 2024-09-04T21:33:59Z

packages/orchestration/test/exos/make-test-loa-kit.ts

-    timer,
-    vowTools,
-    chainHub,
+    {


yeah that was too many positionals

BREAKING CHANGE: lookupDenom -> getDenom and lookupAsset -> getAsset see []Method Naming Structure](https://docs.agoric.com/guides/ertp/#method-naming-structure) for more details

BREAKING CHANGE: The ChainHub.getAsset function returns undefined when a denom key is not found, instead of throwing an error

- getBalance should use asVow, as it synchronously throws errors

- use QueryAllBalancesRequest and localchain.query to return a list of a localOrchAccount's cosmos bank balances

0xpatrickdev force-pushed the pc/local-orch-query-balance branch from 45d1634 to ed3037e Compare September 4, 2024 19:39

0xpatrickdev commented Sep 4, 2024

View reviewed changes

0xpatrickdev requested review from turadg and dckc September 4, 2024 19:40

turadg reviewed Sep 4, 2024

View reviewed changes

0xpatrickdev removed the request for review from dckc September 4, 2024 20:18

0xpatrickdev force-pushed the pc/orch-query-balance branch from 7a9da24 to 11f1cde Compare September 4, 2024 20:42

0xpatrickdev force-pushed the pc/local-orch-query-balance branch from 3dff07c to ac5d742 Compare September 4, 2024 20:46

Base automatically changed from pc/orch-query-balance to master September 4, 2024 21:15

turadg approved these changes Sep 4, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/local-orch-query-balance branch from ac5d742 to 1647cc0 Compare September 4, 2024 22:46

0xpatrickdev added the force:integration Force integration tests to run on PR label Sep 4, 2024

0xpatrickdev force-pushed the pc/local-orch-query-balance branch from 1647cc0 to 2fdcddc Compare September 4, 2024 22:48

0xpatrickdev added the automerge:rebase Automatically rebase updates, then merge label Sep 6, 2024

0xpatrickdev added 11 commits September 6, 2024 03:20

refactor: share toDenomAmount helper

0864699

refactor: move test util out of src

fbe58c7

feat!: lookupDenom -> getDenom and lookupAsset -> getAsset

3ae2d6d

BREAKING CHANGE: lookupDenom -> getDenom and lookupAsset -> getAsset see []Method Naming Structure](https://docs.agoric.com/guides/ertp/#method-naming-structure) for more details

feat!: getAsset returns undefined when lookup fails

5cf8f49

BREAKING CHANGE: The ChainHub.getAsset function returns undefined when a denom key is not found, instead of throwing an error

refactor: prefer denom to denomination in error message

92bb532

fix: localOrchAccount.getBalance

58ed5e7

- getBalance should use asVow, as it synchronously throws errors

feat: localOrchAccount.getBalance queries non-vbank assets

1938ecd

test: export LOCALCHAIN_QUERY_ALL_BALANCES_RESPONSE

518230a

feat: localOrchAccount.getBalances

c95e229

- use QueryAllBalancesRequest and localchain.query to return a list of a localOrchAccount's cosmos bank balances

feat: query-flows fixture supports localchain accounts

72cbd68

test: localOrchAccount.getBalance and localOrchAccount.getBalances

687adf1

0xpatrickdev force-pushed the pc/local-orch-query-balance branch from 2fdcddc to 687adf1 Compare September 6, 2024 03:20

mergify bot merged commit b31bd09 into master Sep 6, 2024
80 checks passed

mergify bot deleted the pc/local-orch-query-balance branch September 6, 2024 03:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: `localOrchAccount.getBalance` (non-vbank assets) and `localOrchAccount.getBalances` #10029

feat: `localOrchAccount.getBalance` (non-vbank assets) and `localOrchAccount.getBalances` #10029

0xpatrickdev commented Sep 4, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Sep 4, 2024 •

edited

Loading

0xpatrickdev Sep 4, 2024

turadg Sep 4, 2024

turadg Sep 4, 2024

turadg Sep 4, 2024

turadg Sep 4, 2024

turadg Sep 4, 2024

turadg Sep 4, 2024

turadg Sep 4, 2024

github-actions bot commented Sep 4, 2024

turadg Sep 4, 2024

turadg Sep 4, 2024

turadg Sep 4, 2024

turadg Sep 4, 2024

turadg Sep 4, 2024

feat: localOrchAccount.getBalance (non-vbank assets) and localOrchAccount.getBalances #10029

feat: localOrchAccount.getBalance (non-vbank assets) and localOrchAccount.getBalances #10029

Conversation

0xpatrickdev commented Sep 4, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Sep 4, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

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

github-actions bot commented Sep 4, 2024

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

feat: `localOrchAccount.getBalance` (non-vbank assets) and `localOrchAccount.getBalances` #10029

feat: `localOrchAccount.getBalance` (non-vbank assets) and `localOrchAccount.getBalances` #10029

0xpatrickdev commented Sep 4, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Sep 4, 2024 •

edited

Loading