-
Notifications
You must be signed in to change notification settings - Fork 4
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
Allow to decode contract call without function name #297
base: master
Are you sure you want to change the base?
Conversation
I'm positive about adding a functionality to decode function id/names, but I definitely disagree with the proposed implementation, I'll comment the code for details. |
src/AciContractCallEncoder.js
Outdated
* @param {string} data - Encoded calldata in canonical format. | ||
* @returns {object} Decoded data | ||
*/ | ||
decodeCallWithoutFunctionName(data) { |
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 prefer to have a single responsibility methods, thus something like decodeFunction
without returning and decoding the actual data
as it already exists as functionality
src/AciTypeResolver.js
Outdated
@@ -140,6 +142,18 @@ class AciTypeResolver extends TypeResolver { | |||
|
|||
return [typeDef, vars] | |||
} | |||
|
|||
getContractAndFunctionNameById(functionId) { |
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.
getFunction
should fit better, have a function name that includes all parameters and return structure properties makes no sense to me, see BytecodeTypeResolver for example
f22b742
to
696b968
Compare
@@ -39,7 +39,7 @@ class AciContractCallEncoder { | |||
* const data = encoder.decodeCall('Test', 'test_string', 'cb_KxHwzCuVGyl3aG9vbHltb2x5zwMSnw==') | |||
* console.log(`Decoded data: ${data}`) | |||
* // Outputs: | |||
* // Decoded data: ["whoolymoly"] | |||
* // Decoded data: { functionId: "f0cc2b95", args: ["whoolymoly"] } |
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.
leftover ?
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.
nope, this example is not correct. I see this format returned in tests:
aepp-calldata-js/tests/AciContractCallEncoder.js
Lines 61 to 83 in 6cb6f2b
test('Decode calldata', t => { | |
t.plan(1) | |
const decoded = encoder.decodeCall( | |
CONTRACT, | |
'test_template_maze', | |
'cb_KxGu5Sw8G6+CAAQBSzsrAgQGCK+EAAABAAIbFCg7KwIEBgj8xaf6', | |
) | |
t.deepEqual( | |
decoded, | |
{ | |
functionId: 'aee52c3c', | |
args: [{ | |
Any: [ | |
{origin: {x: 1n, y: 2n}, a: 3n, b: 4n}, | |
{Yep: [10n]}, | |
20n, | |
{origin: {x: 1n, y: 2n}, a: 3n, b: 4n}, | |
] | |
}] | |
} | |
) | |
}) |
@@ -72,20 +72,43 @@ class AciContractCallEncoder { | |||
* const data = encoder.decodeCall('Test', 'test_string', 'cb_KxHwzCuVGyl3aG9vbHltb2x5zwMSnw==') | |||
* console.log(`Decoded data: ${data}`) | |||
* // Outputs: | |||
* // Decoded data: ["whoolymoly"] | |||
* // Decoded data: { functionId: "f0cc2b95", args: ["whoolymoly"] } |
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.
leftover?
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.
thanks
696b968
to
eafa4cd
Compare
I feel missed the ability to decode contract call without knowing the function name using ACI. The requirement to provide function name looks unnecessary since the calldata already prefixed with the hash of that name.
To avoid ambiguity I propose to limit decoding only to the main contract (the last in ACI).
At last, I don't see a way for good naming without breaking changes.
This PR is supported by the Æternity Foundation