fix(address-hooks): version robustness and hardening #10683
Conversation
michaelfig
force-pushed
the
mfig-no-future-address-hooks
branch
from
2f1fb4a to
8846a0d
Compare
Deploying agoric-sdk with
|
| Latest commit: |
80fee60
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c51e0c3f.agoric-sdk.pages.dev |
| Branch Preview URL: | https://mfig-no-future-address-hooks.agoric-sdk.pages.dev |
michaelfig
force-pushed
the
mfig-no-future-address-hooks
branch
from
8846a0d to
e437a10
Compare
There was a problem hiding this comment.
The hardening and null prototype changes LGTM. I'll let @gibson042 review the versioning changes.
If convenient, consider adding a @throws annotation to decodeAddressHook like b6d20f7. If not, I'll include it in a PR I'm updating to depend on this.
golang/cosmos/types/address_hooks.go
Outdated
| @@ -25,7 +25,7 @@ const ( | |||
|
|
|||
| // AddressHookMagic is a magic byte prefix that identifies a hooked address. | |||
| // Chosen to make bech32 address hooks that look like "agoric10rch..." | |||
| var AddressHookMagic = []byte{0x78, 0xf1, 0x70 | AddressHookVersion} | |||
| var AddressHookMagic = []byte{0x78, 0xf1, 0x70 /* | AddressHookVersion */} | |||
There was a problem hiding this comment.
Suggestion: rename to something AddressHookBytePrefix.
| } | ||
|
|
||
| if (version !== ADDRESS_HOOK_VERSION) { | ||
| return `Unsupported address hook version ${version}`; |
There was a problem hiding this comment.
I didn't get to this function before, but it's bizarre for it to return strings rather than throw exceptions. Please document the reason (or just eliminate the need by e.g. merging it into splitHookedAddress).
...and even if it is important to return rather than throw, consider future-proofing by returning a composite { baseAddress, hookData } | { error } record.
There was a problem hiding this comment.
it's bizarre
Indeed. It's the style that the bech32 package was written in, and it rubbed off on me. :P Time to clean up that muck.
michaelfig
force-pushed
the
mfig-no-future-address-hooks
branch
from
e437a10 to
a8a6172
Compare
michaelfig
added
automerge:no-update
michaelfig
force-pushed
the
mfig-no-future-address-hooks
branch
from
a8a6172 to
80fee60
Compare
closes: #10681
Description
Be more robust for Address Hook version compatibility: return an error if the magic bytes match but the version number is unsupported. If the magic bytes don't match, it is not an address hook, so pass through the specimen as the
baseAddresswith emptyhookData.Also,
hardenthe exports and returned objects if running under HardenedJS.Security Considerations
Stronger Address Hook classification helps prevent accidental collisions. Hardened return objects and exported functions have fewer mutability concerns.
Scaling Considerations
n/a
Documentation Considerations
n/a
Testing Considerations
Some unhappy-path testing has been added to verify at least these issues have been addressed.
Upgrade Considerations
n/a