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(walletFactory): move upgrading check before baggage is populated #8322

Merged
merged 3 commits into from
Sep 14, 2023

Conversation

dckc
Copy link
Member

@dckc dckc commented Sep 11, 2023

closes: #8321
refs: #8311

Description

The baggage.has('walletsByAddress') check has to come before the provideDurableMapStore(baggage, 'walletsByAddress') call.

Testing Considerations

  • IOU a unit test

Meanwhile, @carlos-kryha , if you find a moment to try this fix, please let us know how it goes.

Security / Scaling / Documentation Considerations

none

Upgrade Considerations

This fixes the non-upgrade case, i.e. the 1st incarnation case.

@dckc dckc requested a review from turadg September 11, 2023 16:18
@carlos-kryha
Copy link

It works, no bridge handler error when starting the chain and I'm able to sign transactions via keplr. Thanks for the fix!

Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

Fix looks good. Glad the branch unblocks Kryha.

Requesting changes for code formatting and a regression test. An alternative is to spec and schedule a design that prevents this sort of error.

@@ -142,6 +142,7 @@ export const makeAssetRegistry = assetPublisher => {
* @param {import('@agoric/vat-data').Baggage} baggage
*/
export const prepare = async (zcf, privateArgs, baggage) => {
const upgrading = baggage.has('walletsByAddress');
Copy link
Member

Choose a reason for hiding this comment

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

👌

Any ideas for catching the temptation in the future to move such a check?

@Chris-Hibbert WDYT of the zcf providing this sort of info? Perhaps even incarnationNumber?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think our mock bridge is lax about registration. So I expect that if we make it more strict we could reproduce this in a unit test.

@dckc
Copy link
Member Author

dckc commented Sep 14, 2023

Requesting changes for code formatting and ...

oh no! vs-code has gone rogue! format-on-save was my friend but now it's my enemy.
I wonder how to fix this.

@dckc dckc marked this pull request as ready for review September 14, 2023 00:24
@dckc dckc requested a review from turadg September 14, 2023 00:24
handler = newHandler;
},
setHandler(newHandler) {
handler || Fail`Handler not set`;
Copy link
Member

Choose a reason for hiding this comment

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

eslint giving an error. would it work to use the check style on line 70?

          if (!handler) {
            Fail`No handler for ${bridgeId}`;
          }

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh. I suppose that would have been good for consistency.
I just pushd a fixup with !!handler || Fail... though.

@dckc dckc added the automerge:squash Automatically squash merge label Sep 14, 2023
The `baggage.has('walletsByAddress')` check has to come before the
`provideDurableMapStore(baggage, 'walletsByAddress')` call.
@mergify mergify bot merged commit 97cb715 into master Sep 14, 2023
71 checks passed
@mergify mergify bot deleted the 8321-wf-up-check branch September 14, 2023 18:59
mhofman pushed a commit that referenced this pull request Sep 25, 2023
…8322)

* fix(walletFactory): move upgrading check before baggage is populated

The `baggage.has('walletsByAddress')` check has to come before the
`provideDurableMapStore(baggage, 'walletsByAddress')` call.

* test(smart-wallet): fake bridge manager detects handler not set

... and handler already set

* fixup! test(smart-wallet): fake bridge manager detects handler not set
mhofman added a commit to agoric-labs/KREAd that referenced this pull request Sep 26, 2023
mhofman pushed a commit that referenced this pull request Sep 26, 2023
…8322)

* fix(walletFactory): move upgrading check before baggage is populated

The `baggage.has('walletsByAddress')` check has to come before the
`provideDurableMapStore(baggage, 'walletsByAddress')` call.

* test(smart-wallet): fake bridge manager detects handler not set

... and handler already set

* fixup! test(smart-wallet): fake bridge manager detects handler not set
MangoDream1 pushed a commit to Kryha/KREAd that referenced this pull request Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

walletFactory fails to install bridge handler on first incarnation
3 participants