-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Support encode/decode functions on hosted abi #2348
Conversation
params_ptr: AscPtr<Array<AscPtr<AscEnum<EthereumValueKind>>>>, | ||
) -> Result<AscPtr<Uint8Array>, DeterministicHostError> { | ||
let data = host_exports::abi_encode(self.asc_get(params_ptr)?); | ||
// return `null` if it fails |
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.
This is reasonable, but needs to be documented on the graph-ts
api.
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.
You mean with a | null
in the type annotation?
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.
Ah yes that's even better than just documenting it.
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've added on graph-ts
🙂
I will add later on docs!
88154d8
to
129e381
Compare
runtime/wasm/src/host_exports.rs
Outdated
let param_types = | ||
Reader::read(&types).or_else(|e| Err(anyhow::anyhow!("Failed to read types: {}", e)))?; | ||
|
||
decode(&[param_types], &data).context("Failed to decode") |
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.
If we pass a single type to decode
, we're guaranteed to have a single Token
as output, so we can assert that here and return Token
instead of Vec<Token>
.
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.
Fixed!
129e381
to
c77c4a8
Compare
Only the tests are missing, I can run them locally now, so that should be no problem 🙂 |
Reader::read(&types).or_else(|e| Err(anyhow::anyhow!("Failed to read types: {}", e)))?; | ||
|
||
decode(&[param_types], &data) | ||
.map(|mut tokens| tokens.pop().unwrap()) |
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.
Please add a comment explaining why it's ok to .pop().unwrap()
and discard the vec.
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.
Fixed!
9e2e604
to
75ed3f2
Compare
8d5bb45
to
6a7806c
Compare
assert(address.toAddress() == decoded[0].toAddress(), "address ethereum encoded does not equal the decoded value"); | ||
assert(bigInt1.toBigInt() == decoded[1].toTuple()[0].toArray()[0].toBigInt(), "uint256[0] ethereum encoded does not equal the decoded value"); | ||
assert(bigInt2.toBigInt() == decoded[1].toTuple()[0].toArray()[1].toBigInt(), "uint256[1] ethereum encoded does not equal the decoded value"); | ||
assert(bool.toBoolean() == decoded[1].toTuple()[1].toBoolean(), "boolean ethereum encoded does not equal the decoded value"); |
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 would be good to use some intermediary variables to make the test more readable. It's great to have a test for tuples, could we also test for a simple type, and also for nested tuples? e.g. (address,(address, bool))
.
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.
Done!
6a7806c
to
a9f41fc
Compare
@otaviopace Could you also write something in the 'unreleased' section of NEWS.md about this? |
|
||
let encoded = ethereum.encode(params)!; | ||
|
||
let decoded = ethereum.decode("(address,(address,bool))", encoded).toTuple(); |
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.
The simple case I meant was not a tuple at all, but just .decode("address", ..
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.
Oh okay, fixing!
a9f41fc
to
a2f6905
Compare
317ee3e
to
4f53491
Compare
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.
Great work!
Solves #767.
Note for the reviewer: the part that I am not quite sure if its good is the error handling on
abi_decode
.Also, I need some help with the tests, they should be only on
graph-ts
?