-
Notifications
You must be signed in to change notification settings - Fork 665
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
[NEP-491] Non-refundable storage #9600
Merged
Merged
Changes from 33 commits
Commits
Show all changes
45 commits
Select commit
Hold shift + click to select a range
f363848
NEP-491 reference implementation
jakmeier f101996
reject non-refundable transfer to existing account
jakmeier ab967fe
handle transferV2 in rosetta RPC
jakmeier 75ca826
Merge remote-tracking branch 'upstream/master' into jakmeier/nep-491_…
jakmeier 1b01356
refactor duplicate implicit account creation check
jakmeier 03da556
cfg everything, add mapping to action view
jakmeier 256a873
undo cfg on account version
jakmeier c85d091
fix test compilation errors
jakmeier 1d4d270
reject new action in older protocol versions
jakmeier 0a7fc9d
add nonrefundable field to `AccountView`
jakmeier b9f35ab
fix balance checker
jakmeier 595e39f
add basic transfer_v2 test
jakmeier 65304e9
propagate account view field in conversion
jakmeier 0953f9a
Merge branch 'master' into nep-491_reference_implementation
aa34ee1
Resolve merge issues
a52fdb4
Fix compilation errors
staffik d67e91a
Merge remote-tracking branch 'origin/master' into nep-491_reference_i…
staffik ae36bae
Build fixes
staffik ac4b238
Parse genesis config as it have nonrefundable fields set to 0.
staffik 1492711
Merge remote-tracking branch 'origin/master' into nep-491_reference_i…
staffik f9248d3
Fix nonrefundable_transfer test
staffik 5773e44
Add missing Non-refundable transfer tests
staffik 9c3b741
Update burnt amount on account deletion
staffik d42f125
Eth-implicit non-refundable transfer
staffik 43d56b3
Introduce ReserveStorage action instead of deprecating the Transfer a…
staffik 9e8948c
Fix the mainnet genesis hash issue
staffik b9dddbe
Add comements
staffik 9cfaa00
No need to introduce Account variants
staffik 9ffe5ad
Rename ReserveStorage --> NonrefundableStorageTransfer
staffik 07a1fb4
Merge remote-tracking branch 'origin/master' into nep-491_reference_i…
staffik 65b0175
Minor refactors, update test
staffik 329dcd8
Both refundable and nonrefundable transfers
staffik 2dfc544
Merge remote-tracking branch 'origin/master' into nep-491_reference_i…
staffik ffa1dc4
implicit_account_creation_eligible
staffik 2350f3e
Merge remote-tracking branch 'origin/master' into nep-491_reference_i…
staffik 5cbf7dc
Add comments
staffik 77b47d2
Nit fixes
staffik c43bee3
Disable multiple transfers on implicit account creation
staffik 69a2287
Use protocol version for Account::new()
staffik 11f0b5e
PR fixes
staffik efa4c98
Merge remote-tracking branch 'origin/master' into nep-491_reference_i…
staffik 0b1a097
Return error instead of panicking
staffik 29d0f31
Merge remote-tracking branch 'origin/master' into nep-491_reference_i…
staffik 38a7937
build fixes
staffik f49adef
Add tests
staffik File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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
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
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
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
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
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
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
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 remember checking that this specific behaviour is what we want. I think I've just put it in to have something in the prototype. But considering how the NEP discussion went, I wonder if it wouldn't make more sense to consider the nonrefundable balance burnt instead of showing it as balance on the receiving account?
Can we check with relevant parties how the nonrefundable storage balance should show up in the rosetta RPC?
Ideally, someone who feels responsible for the Rosetta RPC should review all Rosetta RPC changes.
I hope @firatNEAR or @walnut-the-cat can help to find the right person?
The main question to resolve:
When an account has nonrefundable storage balance, should it show up as balance in the Rosetta RPC adapter?
(oh and once this is resolved, I don't think we need to attribute the note to specific person, git blame should work better to find he source of a comment as it leads to the actual PR discussion)
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.
Added a TODO comment to consider this before stabilizing.
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 think this is a question that we should ask Coinbase since they own the spec. They usually are okay with making changes as long as they confirm from their side. @gmilescu could you help me with this, I am a bit out of loop with Coinbase.
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 change should be fine the way Rosetta handles this scenario. Thanks for checking.
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.
@nasmithan Could you take a look at this change?
In discussion we decided to burn the nonrefundable balance instead of showing it as balance on the receiving account.
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.
Yup that should be fine