Updates JSON ABI, LDC, BSIZ, BLDD and ED19. #6254
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Benchmark for b2276f0Click to view benchmark
|
esdrubal
force-pushed
the
esdrubal/abi_changes
branch
2 times, most recently
from
37d49ad to
c3104e4
Compare
Benchmark for b05ffbdClick to view benchmark
|
Benchmark for 20dd3a4Click to view benchmark
|
Benchmark for 0d5b7ffClick to view benchmark
|
4 tasks
Benchmark for e6358f9Click to view benchmark
|
esdrubal
force-pushed
the
esdrubal/abi_changes
branch
from
3ebe4fd to
05c8052
Compare
esdrubal
force-pushed
the
esdrubal/abi_changes
branch
from
98109fa to
efc57dc
Compare
Benchmark for fc4da40Click to view benchmark
|
Benchmark for 4b43436Click to view benchmark
|
esdrubal
force-pushed
the
esdrubal/abi_changes
branch
from
d1be15d to
637d43a
Compare
Benchmark for 9770299Click to view benchmark
|
esdrubal
force-pushed
the
esdrubal/abi_changes
branch
from
c4f2477 to
8924455
Compare
esdrubal
added
the
ABI
esdrubal
force-pushed
the
esdrubal/abi_changes
branch
2 times, most recently
from
f44e59d to
be22465
Compare
Benchmark for 24146f0Click to view benchmark
|
esdrubal
force-pushed
the
esdrubal/abi_changes
branch
2 times, most recently
from
3767f17 to
8971ad1
Compare
Benchmark for 60a1506Click to view benchmark
|
Benchmark for b9b2266Click to view benchmark
|
esdrubal
force-pushed
the
esdrubal/abi_changes
branch
from
53f117e to
2cfcc7e
Compare
Benchmark for f439a33Click to view benchmark
|
Benchmark for 8b8c60bClick to view benchmark
|
esdrubal
added
the
breaking
Benchmark for 263a088Click to view benchmark
|
|
I think we have an issue. TL DRThe following two implementations before and after #[namepsace(SRC14)]
storage {
/// The [ContractId] of the target contract.
///
/// # Additional Information
///
/// `target` is stored at sha256("storage_SRC14_0")
target: Option<ContractId> = None,
/// The [State] of the proxy owner.
///
/// # Additional Information
///
// `proxy_owner` is stored at sha256("storage_SRC14_1")
proxy_owner: State = State::Uninitialized,
}storage {
SRC14 {
/// The [ContractId] of the target contract.
///
/// # Additional Information
///
/// `target` is stored at sha256("storage_SRC14_0")
target: Option<ContractId> = None,
/// The [State] of the proxy owner.
///
/// # Additional Information
///
// `proxy_owner` is stored at sha256("storage_SRC14_1")
proxy_owner: State = State::Uninitialized,
}
}We need to have the proxy contract implementation's abi, storage slots, etc built and embedded in the code for tooling purposes. This used to work until now. Problem is:
Detailed Summary of the problem
#[namepsace(SRC14)]
storage {
/// The [ContractId] of the target contract.
///
/// # Additional Information
///
/// `target` is stored at sha256("storage_SRC14_0")
target: Option<ContractId> = None,
/// The [State] of the proxy owner.
///
/// # Additional Information
///
// `proxy_owner` is stored at sha256("storage_SRC14_1")
proxy_owner: State = State::Uninitialized,
}to storage {
SRC14 {
/// The [ContractId] of the target contract.
///
/// # Additional Information
///
/// `target` is stored at sha256("storage_SRC14_0")
target: Option<ContractId> = None,
/// The [State] of the proxy owner.
///
/// # Additional Information
///
// `proxy_owner` is stored at sha256("storage_SRC14_1")
proxy_owner: State = State::Uninitialized,
}
}
SolutionI also found the solution, basically changing the storage declaration to the following fixes everything: storage {
/// The [ContractId] of the target contract.
///
/// # Additional Information
///
/// `target` is stored at sha256("storage_SRC14_0")
target in 0x7bb458adc1d118713319a5baa00a2d049dd64d2916477d2688d76970c898cd55: Option<ContractId> = None,
/// The [State] of the proxy owner.
///
/// # Additional Information
///
// `proxy_owner` is stored at sha256("storage_SRC14_1")
proxy_owner: State = State::Uninitialized,
}Though with this change I am touching the audited contract, is this ok? #6416 is a dummy PR I opened to show that pinning the storage slot as i described above fixes the issue. |
|
@kayagokalp Very nice find. If I understand it correctly these two should be equal: And updated to: storage {
SRC14 {
/// The [ContractId] of the target contract.
///
/// # Additional Information
///
/// `target` is stored at sha256("storage_SRC14_0")
target in 0x7bb458adc1d118713319a5baa00a2d049dd64d2916477d2688d76970c898cd55: Option<ContractId> = None,
/// The [State] of the proxy owner.
///
/// # Additional Information
///
// `proxy_owner` is stored at sha256("storage_SRC14_1")
proxy_owner in 0xbb79927b15d9259ea316f2ecb2297d6cc8851888a98278c0a2e03e1a091ea754: State = State::Uninitialized,
}
}With the in keyword so the storage fields are backward compatible. What you propose sounds like a perfect solution. |
Benchmark for 0a829ebClick to view benchmark
|
|
This is expected breakage. Which is why we added that syntax that allows you to pick a specific address. The proposed upgrade is exactly what I had in mind. |
IGI-111
approved these changes
Aug 14, 2024
Benchmark for 1a640cdClick to view benchmark
|
sdankel
approved these changes
Aug 14, 2024
Benchmark for 4b943fbClick to view benchmark
|
|
cc @luizstacio to review this change, as I believe he had hexens audit the pre-existing proxy contract. |
|
Just to clarify, I did not pinned owner proxy address like @esdrubal example while building the contract as reading from it is not hardcoded and still works after changing the storage slot keys. To touch the audited contract in a minimal way that still gets things passing and working, I just pinned target field, like I showed in my Solution section. This can also be seen from the storage slots json file. Just wanted to make it clear. So only thing changed is: storage {
/// The [ContractId] of the target contract.
///
/// # Additional Information
///
/// `target` is stored at sha256("storage_SRC14_0")
target in 0x7bb458adc1d118713319a5baa00a2d049dd64d2916477d2688d76970c898cd55: Option<ContractId> = None,
/// The [State] of the proxy owner.
///
/// # Additional Information
///
// `proxy_owner` is stored at sha256("storage_SRC14_1")
proxy_owner: State = State::Uninitialized,
} |
calldelegation
approved these changes
Aug 14, 2024
|
@K1-R1 once this PR is merged we need to add these changes on I understand this needs to be in this way as this is a cycle dependency. As the correct flow would be to update first But from a risk management perspective, as we want to incentivize developers to use our audited proxy. @Voxelot thanks for tagging, we are going to account for the audit diffs. |
Yh I had already prepared the branch with the new storage layout, but I cannot merge it until forc releases with #6366 as it fixes an issue that broke the configurables, and I believe that I will also need FuelLabs/fuels-rs#1475 to be resolved before I can have the tests working for the contract. Then we can re-audit and use that contract in this repo 👍 |
Just to make sure we are on the same page, forc does not fetch anything online related to the proxy, it is embedded in the code in compile time. So unless we open a pr explicitly changing the file content and cut a new release and users migrate to that new release we cannot alter the proxy contract used we designed this such that the bin does not change at all without explicit changes applied |
Yes I'm aware we don't do it right now. The idea is to add such check and use the sway-standard-implementations as the source of true. And remove the manual process for ensuring security. |
Benchmark for 6afa9d4Click to view benchmark
|
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Updates all the dependencies for the current release.
Implements the fuel ABI generation changes proposed in FuelLabs/fuel-specs#599.
Removes the flag
--json-abi-with-callpathsand the behavior is as if it were true. We removed the flag because it is unsafe to produce JSON ABIs without callpaths, so we shouldn't allow it.Includes the LDC, BSIZ, BLDD and ED19 changes from:#6409.
Fixes #5954
Fixes #5151
Checklist
Breaking*orNew Featurelabels where relevant.