-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat!: add support for dynamic configurables #1555
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
base: master
Are you sure you want to change the base?
Conversation
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.
We should probably hold off on merging this until the sway branch catches up with sway master.
I have a feeling it's a bit behind seeing how the configurables offset is 8..16
and not 16..24
seeing as we already support the latter.
Either wait or integrate with the work done in the PR mentioned above so that we detect the compiler version used to generate the sway binary and use 16..24
if it is a modern compiler or 8..16
if it is the older one.
packages/fuels-code-gen/src/program_bindings/abigen/configurables.rs
Outdated
Show resolved
Hide resolved
@@ -51,10 +51,11 @@ mod code_types { | |||
} | |||
} | |||
|
|||
pub(crate) fn code(&self) -> Vec<u8> { | |||
pub(crate) fn code(&self) -> Result<Vec<u8>> { |
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.
Wdyt about having with_code
and with_configurables
return a Result
, trying to apply the old configurables to the new code or the new configurables to the old code.
If that passes then we can .expect
on all the getters and not have Result
everywhere.
Nice consequence is that it will fail closer to the thing that made it fail -- ie the code or the configurables that were invalid, not later on when you try and calculate the contract id.
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.
I like the idea.
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.
In the end you chose to not do it @hal3e ?
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.
improve tests with comments Co-authored-by: Ahmed Sagdati <[email protected]>
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.
I don't understand all the details of each line and interactions but I understood the tests and the main idea and didn't see anything that shocked me. If the tests added pass I think the PR answers the need from an un-educated point of view.
In any cases thank you to not let long time PR die !
@@ -51,10 +51,11 @@ mod code_types { | |||
} | |||
} | |||
|
|||
pub(crate) fn code(&self) -> Vec<u8> { | |||
pub(crate) fn code(&self) -> Result<Vec<u8>> { |
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.
In the end you chose to not do it @hal3e ?
new_enum, | ||
6, | ||
"sway-sway".try_into()?, | ||
"forc".try_into()?, |
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.
Might be nice to explain why "forc" here. I understood that it's the default configurable in the sway contract but it requires a lot of context for this magic word
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.
It is just a random string we encode and decode. It is hard-coded in the binary and defined in the main.sw file
Ok(base_offset_script(consensus_parameters) + padded_len) | ||
} | ||
|
||
pub fn extract_data_offset(binary: &[u8]) -> Result<usize> { | ||
extract_offset_at(binary, 8) |
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.
Might not compile due to type of size_of returns but the idea :
extract_offset_at(binary, 8) | |
extract_offset_at(binary, size_of::<usize>()) |
} | ||
|
||
pub fn extract_offset_at(binary: &[u8], offset: usize) -> Result<usize> { | ||
if binary.len() < offset + 8 { |
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.
Might not compile due to type of size_of returns but the idea :
if binary.len() < offset + 8 { | |
if binary.len() < offset + size_of::<usize>() { |
@@ -3,53 +3,297 @@ pub mod traits; | |||
pub mod types; |
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.
All the functions seems to use a lot of offset in a lot of different orders and modifying some buffers that doesn't make sense for a first-time reader. Can you maybe add some documentation at the top of this file for the module and maybe document the functions you created.
EDIT: The documentation on the tests helps a lot to understands the data we are editing here thanks a lot.
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.
closes: #1481
closes: #1328 as we can read the configurables directly from the binary
BLOCKED until we have a compiler that supports both
abi-errors
anddynamic configurables
.Release notes
In this release, we:
str
configurables.abigen
that can read configurables directly form the binary.ConfigurablesReader
that can be used to read direct and indirect configurables at runtime and compile time.Summary
Breaking Changes
Contract
Regular
andLoader
methodscode()
,contract_id()
,code_root
,state_root
returnResult
Executable
Regular
andLoader
methodscode()
,data_offset_in_code
,loader_code
returnResult
Configurables
'supdate_constants_in
returnsResult
Predicate
'swith_configurable
returnsResult
AbiFormatter
'sdecode_configurables
argumentconfigurable_data
now accepts a&[u8]
Checklist