fix(addAssetToVault): support issuerName separate from keyword #8229

dckc · 2023-08-21T22:51:05Z

Description

In addAssetToVault, we conflated keyword (needed for registration with the reserve and auction) with issuerName (for use in agoricNames). This adds to InterchainAssetOptions an issuerName property that defaults to the value of keyword.

Security, Documentation Considerations

Allows higher fidelity between agoricNames and well-known names of assets.

Scaling Considerations

n/a/

Testing Considerations

Manual testing with issuerName: 'atom' and issuerName: 'stars' succeeded. We could keep the issuerName: 'stars' tweak. But first, I'd like to see what ci thinks of this.

Upgrade Considerations

doesn't affect existing vats.

turadg · 2023-08-21T22:58:33Z

packages/inter-protocol/src/proposals/addAssetToVault.js

+ * @property {string} [issuerName]
 * @property {string} oracleBrand


why allow oracleBrand to be different than issuerName?

I don't recall. Perhaps no reason. I suppose the thought at the time of writing was: why constrain them to be the same?

@michaelfig ? maybe you have a more clear idea of the constraints?

oh... are you suggesting that oracleBrand suffices instead of adding an issuerName property? I suppose that could work, though it seems like a stretch.

if they don't truly need to be different my suggestion would be to rename the oracleBrand property to issuerName.

Whether they need to be different… they are different brands but I think it's a source of unnecessary confusion to have different strings, since their different namespaces suffice.

My 2c - in the event there were ever multiple oracle options for a single asset (say, redemption rate, twap, etc), provided as separate PriceAuthorities, it seems like it'd be beneficial to have issuerName and oracleBrand decoupled

0xpatrickdev · 2023-08-22T15:44:29Z

Do we also need to update /scripts/add-collateral-core.js? If I specify an issuerName in interchainAssetOptions in /scripts/add-STARS.js, it is not propagated through to the getManifestCall object in the generated script

@dckc

…#8229 - @dckc

dckc · 2023-08-22T17:09:42Z

@0xpatrickdev did some testing and found a problem

2023-08-22T15:54:16.291Z SwingSet: xsnap: v29: Error: keyword "stATOM" must be an ascii identifier starting with upper case.
 at construct ()
...
 at In "assertUniqueKeyword" method of (InstanceRecord) (/bundled-source/.../node_modules/@endo/exo/src/exo-tools.js:42)
 at saveIssuer (.../zoe/src/contractFacet/zcfZygote.js:261)
 at saveIssuer (.../zoe/src/contractFacet/zcfZygote.js:257)
 at updateState (.../vats/src/provisionPoolKit.js:344)
 at updateState (.../vats/src/provisionPoolKit.js:342)

Looks like we have more keyword/issuerName conflation in provisionPool. :-/

https://github.com/Agoric/agoric-sdk/blob/mainnet1B/packages/vats/src/provisionPoolKit.js#L347

p.s. for follow-up, see #8238

@dckc

…#8229 -@dckc

@dckc

…#8229 -@dckc

@dckc

…#8229 @dckc

0xpatrickdev · 2023-10-03T14:56:35Z

packages/inter-protocol/src/proposals/addAssetToVault.js

 * @property {string} [proposedName]
 * @property {string} keyword
- * @property {string} oracleBrand
+ * @property {string} [issuerName]
+ * @property {string} [oracleBrand]


The new changes since last testing period look good to me (note: just from reading the code, i have yet to try a deployment). I might suggest adding these comments, or something along these lines, to add more clarity around the parameters:

Suggested change

* @property {string} [proposedName]

* @property {string} keyword

* @property {string} oracleBrand

* @property {string} [issuerName]

* @property {string} [oracleBrand]

* @property {string} keyword - the issuer keyword record for the contract instance

* @property {string} [issuerName] - defaults to `keyword` if not provided

* @property {string} [proposedName] - defaults to `issuerName` if not provided

* @property {string} [oracleBrand] - defaults to `issuerName` if not provided

(keyword took me a minute to grok, but it's seemingly just for the use case i described)

turadg

No blockers but please consider the suggestions before merging.

turadg · 2023-10-03T15:44:14Z

packages/vats/src/mintHolder.js

@@ -10,6 +10,8 @@ import {
 /** @typedef {import('@agoric/vat-data').Baggage} Baggage */

 /**
+ * NOTE: "keyword" connotes initial caps constraint, which doesn't apply here.


consider explaining why the imperfect name remains

Hm. I considered it, but I don't see a concise explanation.

Changing the name wouldn't require upgrading the existing contract vats. It would just require remembering that the old ones use a different name. It might even be possible to support a new name property and deprecate the keyword property.

I suppose the real reason the keyword name remains is: changing it isn't necessary to address #8235.

require remembering that the old ones use a different name
That seems reason enough to me.

the keyword name remains is: changing it isn't necessary

Fair. Though with this PR that imperfect name is tech debt. We should at least document the rationale to take on the debt.

turadg · 2023-10-03T15:48:52Z

packages/inter-protocol/src/proposals/addAssetToVault.js

+    keyword: kwd,
+    issuerName = kwd,
+    oracleBrand = issuerName,


You have a nice and clear progressive override below. Why not use it here?

Suggested change

keyword: kwd,

issuerName = kwd,

oracleBrand = issuerName,

keyword,

issuerName = keyword,

oracleBrand = issuerName,

turadg · 2023-10-03T15:50:03Z

packages/inter-protocol/src/proposals/addAssetToVault.js

  assert.typeof(proposedName, 'string');
+  assert.typeof(decimalPlaces, 'number');


moving this line looks superfluous to me. If you are going to move it, consider some rational ordering like alphabetical (as you have in the destructuring for this fn

github-actions · 2023-10-05T08:00:44Z

@dckc - This PR appears to be stuck. It's had a merge label for > 24 hours

- default proposedName to issuerName The type for proposedName is optional but there was a runtime typecheck for string. - default oracleBrand to issuerName

mhofman · 2023-10-05T19:05:15Z

@Mergifyio refresh

mergify · 2023-10-05T19:05:21Z

refresh

✅ Pull request refreshed

dckc requested a review from turadg August 21, 2023 22:51

turadg reviewed Aug 21, 2023

View reviewed changes

dckc mentioned this pull request Aug 21, 2023

stATOM vs STATOM in agoricNames.brand 0xpatrickdev/agoric-vault-collateral-proposal#16

Closed

dckc force-pushed the dc-interchain-name-case branch from d69413d to 38edd99 Compare August 22, 2023 15:50

0xpatrickdev added a commit to 0xpatrickdev/agoric-sdk that referenced this pull request Aug 22, 2023

fix(addAssetToVault): support issuerName separate from keyword Agoric…

90e8ce1

…#8229 - @dckc

dckc mentioned this pull request Aug 22, 2023

addAssetToVault does not support initial lower-case asset names #8235

Closed

0xpatrickdev added a commit to 0xpatrickdev/agoric-sdk that referenced this pull request Aug 22, 2023

fix(addAssetToVault): support issuerName separate from keyword Agoric…

99bd000

…#8229 -@dckc

0xpatrickdev added a commit to 0xpatrickdev/agoric-sdk that referenced this pull request Aug 22, 2023

fix(addAssetToVault): support issuerName separate from keyword Agoric…

a884541

…#8229 -@dckc

0xpatrickdev added a commit to 0xpatrickdev/agoric-sdk that referenced this pull request Aug 22, 2023

fix(addAssetToVault): support issuerName separate from keyword Agoric…

3528e89

…#8229 @dckc

0xpatrickdev mentioned this pull request Aug 22, 2023

feat: lowercase brand issuer name 0xpatrickdev/agoric-vault-collateral-proposal#17

Merged

dckc force-pushed the dc-interchain-name-case branch from 2b75c1b to 8fc0c23 Compare August 22, 2023 21:46

dckc force-pushed the dc-interchain-name-case branch 2 times, most recently from 9600d4e to 7b29446 Compare October 3, 2023 01:06

dckc marked this pull request as ready for review October 3, 2023 01:06

dckc requested review from 0xpatrickdev and turadg October 3, 2023 01:07

0xpatrickdev reviewed Oct 3, 2023

View reviewed changes

turadg approved these changes Oct 3, 2023

View reviewed changes

dckc added the automerge:rebase Automatically rebase updates, then merge label Oct 3, 2023

dckc force-pushed the dc-interchain-name-case branch from 65c0d76 to 54e5094 Compare October 5, 2023 17:36

dckc added 4 commits October 5, 2023 13:47

docs: scaledPriceAuthority was missing from well-known installations

eb563d3

docs: note keyword misnomer in mintHolder

42b3d4e

fix(addAssetToVault): support issuerName separate from keyword

a670616

- default proposedName to issuerName The type for proposedName is optional but there was a runtime typecheck for string. - default oracleBrand to issuerName

fix: support issuerName separate from keyword in add-collateral-core

f0b1559

dckc force-pushed the dc-interchain-name-case branch from 54e5094 to f0b1559 Compare October 5, 2023 18:47

mergify bot merged commit 3fd7a2b into master Oct 5, 2023
71 checks passed

mergify bot deleted the dc-interchain-name-case branch October 5, 2023 19:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(addAssetToVault): support issuerName separate from keyword #8229

fix(addAssetToVault): support issuerName separate from keyword #8229

dckc commented Aug 21, 2023 •

edited

Loading

turadg Aug 21, 2023

dckc Aug 21, 2023

dckc Aug 21, 2023

turadg Aug 22, 2023

0xpatrickdev Aug 22, 2023

0xpatrickdev commented Aug 22, 2023

dckc commented Aug 22, 2023 •

edited

Loading

0xpatrickdev Oct 3, 2023 •

edited

Loading

turadg left a comment

turadg Oct 3, 2023

dckc Oct 3, 2023

turadg Oct 3, 2023

turadg Oct 3, 2023

turadg Oct 3, 2023

github-actions bot commented Oct 5, 2023

mhofman commented Oct 5, 2023

mergify bot commented Oct 5, 2023

		* @property {string} [issuerName]
		* @property {string} oracleBrand

		assert.typeof(proposedName, 'string');
		assert.typeof(decimalPlaces, 'number');

fix(addAssetToVault): support issuerName separate from keyword #8229

fix(addAssetToVault): support issuerName separate from keyword #8229

Conversation

dckc commented Aug 21, 2023 • edited Loading

Description

Security, Documentation Considerations

Scaling Considerations

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xpatrickdev commented Aug 22, 2023

dckc commented Aug 22, 2023 • edited Loading

0xpatrickdev Oct 3, 2023 • edited Loading

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

github-actions bot commented Oct 5, 2023

mhofman commented Oct 5, 2023

mergify bot commented Oct 5, 2023

✅ Pull request refreshed

dckc commented Aug 21, 2023 •

edited

Loading

dckc commented Aug 22, 2023 •

edited

Loading

0xpatrickdev Oct 3, 2023 •

edited

Loading