Skip to content
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

Specify deposit IDs #314

Merged
merged 8 commits into from
May 19, 2022
Merged

Specify deposit IDs #314

merged 8 commits into from
May 19, 2022

Conversation

pixelcircuits
Copy link
Contributor

Updated deposit ID compute process

updated deposit ID compute process
@pixelcircuits
Copy link
Contributor Author

Not sure exactly what we want to make up the deposit ID. Technically the depositNonce would be enough, but I figured it might be useful to hash in other data bits to be able to more easily prove their existence if not immediately accessible in smart contracts / scripts.

Also, please advise where else this would need to be specified. I see #103 added to the tx_format part of the protocol to specify withdrawal outputs, but I'm not sure what the parallel would be for deposits.

@adlerjohn @simonr0204

Copy link
Contributor

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can leave specifying a deposit state element (similar to a UTXO) to a future PR, and only specify the actual identifier for a deposit in this PR.

specs/protocol/identifiers.md Outdated Show resolved Hide resolved
@adlerjohn adlerjohn changed the title Update identifiers.md Specify deposit IDs May 13, 2022
Copy link
Contributor

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Lots of stuff in the FuelVM works best at word alignment (i.e. 8 bytes). I would instead use 32 bytes for addresses, and left-pad with zeroes (which is what FuelLabs/sway#1452) does. This would also be forwards-compatible with the proposed address extension: https://ethereum-magicians.org/t/thoughts-on-address-space-extension-ase/6779

Also, is there a need for 32 byte nonces? Would prefer 8 bytes (i.e. uint64) since it'll just be easier to deal with in the FuelVM.

specs/protocol/identifiers.md Outdated Show resolved Hide resolved
@pixelcircuits
Copy link
Contributor Author

Changed address to be 32 bytes left-padded with zeroes and deposit nonce reduced to 64bits (still plenty)

adlerjohn
adlerjohn previously approved these changes May 15, 2022
@adlerjohn adlerjohn requested review from simonr0204 and rakita May 15, 2022 02:43
@adlerjohn
Copy link
Contributor

Looping in @simonr0204 and @rakita for reviews from the perspective of the contract and the relayer, respectively.

@simonr0204
Copy link
Contributor

simonr0204 commented May 15, 2022

How are we defining precision in the general case? For the current ethereum bridges it's relative to 18 decimal places, since ether and many ERC20 tokens have 18 digits of precision.

Do we want to be stuck with this definition for non-evm deployments ?

Also - is the address the depositor (on L1) , or the Fuel address receiving the deposit?

@adlerjohn
Copy link
Contributor

adlerjohn commented May 15, 2022

Good questions. The precision here should be against units, e.g. wei for Ethereum. So a precision of 3 would mean 1,000 wei. Does that work? Potentially a different name is needed.

As for address, it should be the receiving address on Fuel, which is 32 bytes so no padding necessary. The sending address is irrelevant for the Fuel chain.

@simonr0204
Copy link
Contributor

Makes sense. I'll update the contracts to reflect.

For the address, depositor may be a bit misleading then, as it sounds like it's referring to the L1 address initiating the deposit. Is there a better word? Receiver? Beneficiary?

@pixelcircuits
Copy link
Contributor Author

Maybe we refer to depositor as recipient instead?

@adlerjohn
Copy link
Contributor

Recipient ain't bad

@rakita
Copy link
Contributor

rakita commented May 16, 2022

Looping in @simonr0204 and @rakita for reviews from the perspective of the contract and the relayer, respectively.

Relayer does not care what is inside of hash, we just need some kind of id, only having nonce would be sufficient enough for our side.

Regarding deposits, in fuel ecosystem we don't have u256, we are using u64 as an amount, so it is best to restrict max deposit amount to u64::MAX.

I am not exactly getting how precision is going to work inside FuelVm, what is the idea behind it is this some data that is going to be used by VM or something like additional info that is tied to coin?

@adlerjohn
Copy link
Contributor

I am not exactly getting how precision is going to work inside FuelVm, what is the idea behind it is this some data that is going to be used by VM or something like additional info that is tied to coin?

The FuelVM won't actually use the precision. Token contracts can be provided the preimage to the ID to extract the precision, then use that to mint an appropriate number of Fuel-tokens.

Copy link
Contributor

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, just remembered we need a notion of asset ID for deposits. This should be independent of the user, the amount, and the nonce. Should be computed as token address + token precision.

Then you can use this asset ID in the deposit ID computation instead of duplicating usage of the token address and precision

specs/protocol/identifiers.md Outdated Show resolved Hide resolved
specs/protocol/identifiers.md Outdated Show resolved Hide resolved
specs/protocol/identifiers.md Outdated Show resolved Hide resolved
@pixelcircuits pixelcircuits marked this pull request as ready for review May 17, 2022 00:09
@pixelcircuits pixelcircuits merged commit 0d56640 into master May 19, 2022
@pixelcircuits pixelcircuits deleted the circuitpixels-specify-deposit-id branch May 19, 2022 02:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants