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

fix(cosmic-swingset): Publish installation success and failure topic #5541

Merged
merged 2 commits into from
Sep 8, 2022

Conversation

kriskowal
Copy link
Member

@kriskowal kriskowal commented Jun 8, 2022

closes: #5460

Description

Deploy scripts can now publish bundles directly to the chain. Currently, they receive no feedback as to whether the message was adopted into the chain, much less dequeued and successfully processed. This change will create an on-chain publication that the off-chain deploy script can subscribe to, to learn of the fate of their attempted installation. The deploy script machinery will then watch for success, failure, or timeout. If the installation times out, it will retry.

Security Considerations

Documentation Considerations

Testing Considerations

For manual verification, in dapp-fungible-faucet:

# tab 1
agoric start local-chain --reset

# tab 2
agoric start local-solo --reset

# tab 3
agoric follow :published.installation

# tab 4
agoric deploy contract/deploy.js --need=agoric

@kriskowal kriskowal force-pushed the 5460-publish-bundle branch from def73a3 to 713b472 Compare June 11, 2022 01:21
@kriskowal
Copy link
Member Author

We got publishing to work, huzzah.

@kriskowal kriskowal marked this pull request as ready for review June 11, 2022 01:24
@kriskowal kriskowal requested a review from gibson042 June 11, 2022 01:24
@kriskowal kriskowal force-pushed the 5460-publish-bundle branch 2 times, most recently from a3290d9 to 560af97 Compare June 13, 2022 23:44
packages/cosmic-swingset/src/chain-main.js Show resolved Hide resolved
packages/cosmic-swingset/src/block-manager.js Outdated Show resolved Hide resolved
packages/cosmic-swingset/src/block-manager.js Outdated Show resolved Hide resolved
* @param {StorageNode} [storageNode]
*/
const makeStoredPublisher = (initialValue, storageNode) => {
let providedPublication;
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason not to initialize more fully up front? Setting up the dependencies should be inexpensive... is it instead a case of chainStorage writes happening too soon (and if so, is that fixed by switching to makeEmptyPublishKit(), which has no initial value)?

Suggested change
let providedPublication;
const { publication, subscription } = makeSubscriptionKit(initialValue);
makeStoredSubscription(subscription, storageNode);
return { publish: publication.updateState };

Copy link
Member Author

Choose a reason for hiding this comment

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

In #6103, I made casting more resilient to uninitialized storage, but at this height of the PR stack, the {coming: 'soon'} value substantially greases the wheels.

packages/cosmic-swingset/src/block-manager.js Outdated Show resolved Hide resolved
packages/cosmic-swingset/src/block-manager.js Outdated Show resolved Hide resolved
@kriskowal kriskowal force-pushed the 5460-publish-bundle branch from 560af97 to c7f4730 Compare June 23, 2022 20:22
@kriskowal kriskowal force-pushed the 5460-publish-bundle branch 3 times, most recently from 908cedc to 002b652 Compare July 26, 2022 23:52
@kriskowal kriskowal force-pushed the 5460-publish-bundle branch from 002b652 to 3edb6ab Compare August 24, 2022 01:47
@kriskowal kriskowal changed the base branch from master to 5601-connection-refused August 24, 2022 01:47
@kriskowal kriskowal force-pushed the 5601-connection-refused branch from a4f15ea to 806f41c Compare August 24, 2022 01:58
@kriskowal kriskowal force-pushed the 5460-publish-bundle branch from 3edb6ab to 061f702 Compare August 24, 2022 01:58
@kriskowal kriskowal force-pushed the 5601-connection-refused branch from 806f41c to e2ec5ff Compare August 24, 2022 21:20
@kriskowal kriskowal force-pushed the 5460-publish-bundle branch from 061f702 to 9d7d352 Compare August 24, 2022 21:20
@kriskowal kriskowal force-pushed the 5601-connection-refused branch from e2ec5ff to b6aa71d Compare August 31, 2022 00:00
@kriskowal kriskowal force-pushed the 5460-publish-bundle branch from 9d7d352 to 169ea3f Compare August 31, 2022 00:00
@kriskowal kriskowal force-pushed the 5601-connection-refused branch from b6aa71d to 3085c84 Compare August 31, 2022 00:35
@kriskowal kriskowal force-pushed the 5460-publish-bundle branch from 169ea3f to 2db6122 Compare August 31, 2022 00:35
@kriskowal kriskowal force-pushed the 5601-connection-refused branch from 3085c84 to f925894 Compare August 31, 2022 00:48
@kriskowal kriskowal force-pushed the 5460-publish-bundle branch from 2db6122 to 152b695 Compare August 31, 2022 00:48
@kriskowal kriskowal force-pushed the 5601-connection-refused branch from f925894 to 260cd69 Compare August 31, 2022 00:49
@kriskowal kriskowal force-pushed the 5460-publish-bundle branch from 152b695 to 67e9a7d Compare August 31, 2022 00:49
@kriskowal kriskowal force-pushed the 5601-connection-refused branch from 260cd69 to 2df40ac Compare August 31, 2022 00:52
@kriskowal kriskowal force-pushed the 5460-publish-bundle branch from 67e9a7d to 9a77b11 Compare August 31, 2022 00:52
@kriskowal kriskowal force-pushed the 5601-connection-refused branch from 2df40ac to 3ea06d4 Compare August 31, 2022 00:53
@kriskowal kriskowal force-pushed the 5601-connection-refused branch from e752371 to 44049b3 Compare September 1, 2022 21:46
@turadg turadg force-pushed the 5601-connection-refused branch from 44049b3 to 908f723 Compare September 3, 2022 18:57
Base automatically changed from 5601-connection-refused to master September 3, 2022 19:37
Copy link
Member

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

Looks good after a few tweaks; let me know if you think anything needs discussion.

@@ -146,27 +174,42 @@ export const makeHttpBundlePublisher = ({ jsonHttpCall, getAccessToken }) => {
return publishBundleHttp;
};

/**
* @param {string} address - a host or URL
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param {string} address - a host or URL
* @param {string} address - a domain name, IPv4 address, or URL
* @returns {string} the corresponding URL, defaulting scheme to http

Note that the current implementation will not work for IPv6 addresses because those must be wrapped in square brackets to be used as URL hosts (e.g., http://[2001:db8::1]/ rather than http://2001:db8::1/): https://www.rfc-editor.org/rfc/rfc3986#section-3.2.2

This also seems like the kind of thing that should really be written once [per language and cross-referenced] and exposed in a library, but I don't consider that a blocking concern for this PR.

For the record, the other places I see similar code:

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed wrt consolidating the implementations. It was not trivial to discover the prevailing idiom for this.

/**
* @template T
* @param {Array<T>} array
* @param {number} randomNumber
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param {number} randomNumber
* @param {number} randomNumber expected to be in [0, 1)

const { publisher, subscriber } = makePublishKit();
makeStoredSubscriber(subscriber, installationStorageNode, marshaller);
installationPublisher = publisher;
publisher.publish({ coming: 'soon' });
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
publisher.publish({ coming: 'soon' });
// Publish an inconsequential initial value.
publisher.publish({ coming: 'soon' });

Copy link
Member Author

Choose a reason for hiding this comment

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

This may not be necessary after #6103 and I’ll make a note.

packages/cosmic-swingset/src/chain-main.js Outdated Show resolved Hide resolved
@kriskowal
Copy link
Member Author

Ready for another round, pending CI. Follow-up commits not yet squashed and individually reviewable.

@kriskowal kriskowal force-pushed the 5460-publish-bundle branch 2 times, most recently from 9d0dd23 to 46713d2 Compare September 7, 2022 00:07
Copy link
Member

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

LGTM after updating golang/cosmos/x/swingset/keeper/keeper.go .

@@ -9,4 +9,5 @@ export const ACTIVITYHASH = 'activityhash';
export const BEANSOWING = 'beansOwing';
export const EGRESS = 'egress';
export const MAILBOX = 'mailbox';
export const BUNDLES = 'bundles';
Copy link
Member

Choose a reason for hiding this comment

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

Please also add an analogous constant to golang/cosmos/x/swingset/keeper/keeper.go to preserve synchronization as noted in the introductory comment.

@kriskowal kriskowal added the automerge:rebase Automatically rebase updates, then merge label Sep 7, 2022
@kriskowal kriskowal force-pushed the 5460-publish-bundle branch 8 times, most recently from fd2f783 to 29901be Compare September 8, 2022 02:09
@turadg turadg force-pushed the 5460-publish-bundle branch from 29901be to 02ac897 Compare September 8, 2022 03:26
@kriskowal kriskowal added automerge:rebase Automatically rebase updates, then merge and removed automerge:rebase Automatically rebase updates, then merge labels Sep 8, 2022
@mergify mergify bot merged commit 158f5c4 into master Sep 8, 2022
@mergify mergify bot deleted the 5460-publish-bundle branch September 8, 2022 17:22
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chain ACKs publish bundle
2 participants