-
Notifications
You must be signed in to change notification settings - Fork 208
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(provisioning): export useful namesByAddress (release branch) #8786
Conversation
seems to work... I wonder what all the failed deposit to stATOM-USD price feed member are about. I hope it's left over from experimenting with other things on this local chain.
|
I reviewed all occurrences of |
c3d7a60
to
2d9b00c
Compare
2d9b00c
to
0b3d941
Compare
`namesByAddress` supplied by the provisioning vat was an independent NameHub from `namesByAddressAdmin`.
6855580
to
6999bff
Compare
6999bff
to
7286ca1
Compare
7286ca1
to
3d4e9af
Compare
- run core-eval to replace provisioning vat - test incarnation - functional test
3d4e9af
to
e5a2c17
Compare
@turadg please take a look. It's draft only because it's stacked on #8819 , my work-in-progress port of a3p-integration with "simplify core-eval" to a release branch context. @mhofman perhaps you've got thoughts on how this should land in the release branch. cc @Chris-Hibbert who is doing similar stuff. See also #8821 for the corresponding fix for master, especially the testing considerations. |
return { txhash, code, height, gas_used }; | ||
}; | ||
|
||
// XXX overlaps with passCoreEvalProposal from synthetic chain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm... the reason has faded. something about staticConfig? I'll have to look again.
#!/bin/bash | ||
source /usr/src/upgrade-test-scripts/env_setup.sh | ||
|
||
yarn ava |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
glad see no file specified
@@ -67,6 +67,16 @@ test('sim/demo config provides home with .myAddressNameAdmin', async t => { | |||
keyArrayEqual(t, keys(home).sort(), homeKeys); | |||
}); | |||
|
|||
test('namesByAddress contains provisioned account', async t => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good thinking including here too
overtaken by stuff such as ##8901 |
closes: #8311
this fix is for the release branch, anyway.
see #8821 for fix on master
DRAFT until:
namesByAddress
Description
The
namesByAddress
provided by the provisioning vat is disconnected fromnamesByAddressAdmin
, so it never gets populated.Fortunately, the relevant
nameHubKit
is in baggage. So the fix here is to return that one.Security Considerations
This should make it unnecessary for future core-eval scripts to get the whole
namesByAddressAdmin
authority when they only neednamesByAddress
to do something like deliver fees.The permit of the upgrade core-eval is:
Read/write access to the whole
vatStore
is excess authority; we only need access to read one of its entries, not authority to scribble over all the others and upgrade all the other vats.vatAdminSvc
is needed to look up a bundleID (hash) and return the corresponding bundleCap. Its only method beyond that sort of bundle lookup isE(vatAdminSvc).createVat()
. That method is more than we need, but while it can be wasteful, it doesn't pose any risk to integrity.Scaling Considerations
n/a
Documentation Considerations
fixes the chain to agree with our docs on name services
Testing Considerations
see DRAFT until list above
Upgrade Considerations
The bug is localized to the provisioning vat, so upgrading it seems to suffice.
Perhaps we should fix #8408 while we're at it?